CI improvements #4307
Open
CI improvements #4307
+158
−127
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ptomato
force-pushed
the
ci-improvements
branch
9 times, most recently
from
fd8e07a
to
172e00b
Compare
|
Screenshot of what the linter annotations look like: |
ljharb
reviewed
Nov 5, 2024
Ms2ger
approved these changes
Nov 6, 2024
.github/workflows/checks-pr.yml
Outdated
| if: failure() | ||
| run: | | ||
| awk -F': ' <errors.out \ | ||
| "{ sub(\"$(pwd)/\", \"\"); printf \"::error file=%s,line=1::%s\n\", \$1, \$2 }" |
There was a problem hiding this comment.
Please file a followup issue to see if we can easily output line numbers
ptomato
force-pushed
the
ci-improvements
branch
2 times, most recently
from
2bc86f9
to
f33d276
Compare
Applying only to .travis.yml seems to be a relic from when that was the only YAML file in the repo. Extend it to all YAML files.
We do not use these anymore. Generated tests are included in PRs and not deployed separately. The deploy key encryption relies on TravisCI which we don't use anymore, anyway.
This runs faster and allows for future improvements. I'm following a general principle of keeping code that isn't portable between CI providers inside the config file for the CI provider. So in this case we remove the Circle-CI-specific stuff from the file in tools/scripts/, and into .github/workflows/. We use an external action (tj-actions/changed-files) to gather the list of files to lint.
After the previous commit, it doesn't do much, only passes the linting exceptions file on the command line. Instead, make the lint.exceptions file the default for that argument (if it exists), and remove the shell script.
This should help contributors notice and fix linter errors when they occur. Apparently it's as easy as printing a magic string to stdout, who knew! The lint tool doesn't give line numbers so we place the annotations at line 1 for the time being.
This runs faster and allows for future improvements. Similar to the linter workflow, we remove the Circle-CI-specific stuff from the file in tools/scripts/, and into .github/workflows/. We use another external action (tj-actions/verify-changed-files) to check that generating the tests didn't create any new changes. This is basically a companion action to the tj-actions/changed-files used in the linter job.
After the previous commit, it doesn't do much. Just write its contents directly in the workflow step.
Most of the time, we are not committing changes to the tools. Move the unit tests for the lint and generation tools to a separate PR workflow, that is only run if anything in the tools/ folder is modified in the PR. This saves time in the normal case.
Since we are using it in the other jobs, we may as well use it here. As a bonus, it will make the job work even if the target of the pull request isn't the main branch.
This reduces duplication, with the tradeoff of having some steps in the linter job that never get executed in one or the other situation.
ptomato
force-pushed
the
ci-improvements
branch
from
f33d276
to
fa8c451
Compare
ljharb
approved these changes
Nov 8, 2024
Comment on lines
+4
to
+6
| - push: | ||
| branches: | ||
| main |
There was a problem hiding this comment.
any reason not to run this on any branch push?
There was a problem hiding this comment.
It seems to me that only the pull_request checks are prominently visible in the UI and are likely to be noticed. I'd still like to have them run on pushes to main so that we can monitor if anything incorrect is merged, so that's why I added the push trigger. But otherwise in the usual case where you push a branch and open a PR almost immediately, we'd just be running a job that no-one will see.
That said, I'm not opposed to this; just think it's unnecessary.
| name: Test tools | ||
|
|
||
| on: | ||
| pull_request: |
There was a problem hiding this comment.
Suggested change
| pull_request: | |
| push: | |
| pull_request: |
Ms2ger
approved these changes
Nov 8, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Migrate linter and test generation workflow to GitHub Actions, and add some improvements
To merge this, we'll have to go into the settings, set the "ci/circleci: Test262: verify tools; build & lint tests" workflow to non-required, merge it, and then go back to the settings and set whichever new workflows we'd like to require.