Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize PR checks #13032

Merged
merged 1 commit into from
Jul 12, 2024
Merged

Optimize PR checks #13032

merged 1 commit into from
Jul 12, 2024

Conversation

mlqn
Copy link
Contributor

@mlqn mlqn commented Jun 26, 2024

Description

Here is the list of optimizations made on PR checks:

  • Removed build-image-on-pr.yaml workflow since Docker images are already built by Playwright tests
  • Removed Building job from frontend-unit-tests.yml workflow as the solution is already built by Playwright tests
  • Removed Build all images job from run-playwright-on-pr.yaml since the solution is already built during setup
  • Removed Stop compose file job from run-playwright-on-pr.yaml as it's already stopped when the VM is stopped
  • Removed dotnet build from Dockerfile as the backend is already built by dotnet publish
  • Fixed the typecheck command to prevent it from running multiple times. This is a quick fix, we should fix the tsconfig.json files to make the typecheck command work per workspace (see: Fix typecheck command #13031)
  • Optimized Dockerfile layer caching by separating the copying of package.json and yarn.lock files
  • Added a concurrency control on run-playwright-on-pr.yaml to stop previous running instances
  • Added the --cache flag to the eslint command to only check changed files
  • Added the --changedSince flag to the Jest command to only run tests impacted by changes in a PR
    We still run all the tests when merging a PR into main, as it seems that sometimes this command does not execute all the impacted files.

There are more optimizations that can be made, particularly regarding caching during the installation of Playwright, but this requires more work, so I have created a separate issue for that (see: #13028).

PR checks

BEFORE AFTER

before

after

Docker layer caching

BEFORE AFTER
before after

Related Issue(s)

Verification

  • Your code builds clean without any errors or warnings
  • Manual testing done (required)

@github-actions github-actions bot added the skip-releasenotes Issues that do not make sense to list in our release notes label Jun 26, 2024
@mlqn mlqn mentioned this pull request Jun 26, 2024
2 tasks
@mlqn mlqn marked this pull request as ready for review June 26, 2024 11:33
Copy link
Collaborator

@framitdavid framitdavid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! Just some comments/questions to discuss with you. 😊

package.json Show resolved Hide resolved
.github/workflows/run-playwright-on-pr.yaml Show resolved Hide resolved
.github/workflows/build-image-on-pr.yaml Show resolved Hide resolved
.github/workflows/frontend-unit-tests.yml Show resolved Hide resolved
Copy link
Collaborator

@framitdavid framitdavid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 😊 I approve this now as I'm heading off for holidays. I understand that you are working on separating the build and sharing it between workflows. You can decide whether to address that in this PR or create a separate issue for it. 💪

@mlqn mlqn merged commit f7c177b into main Jul 12, 2024
6 checks passed
@mlqn mlqn deleted the 12474-remove-duplicated-pr-checks-3 branch July 12, 2024 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-releasenotes Issues that do not make sense to list in our release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove duplicated PR checks Caching yarn packages when building the Docker image
2 participants