-
Notifications
You must be signed in to change notification settings - Fork 3
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
GH Actions: various updates to the workflows #315
Merged
Merged
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
Note: this PR needs a one-time only change in the branch protection settings to account for the new build names. |
... as the can't be fixed on `main` anyway. This would need a PR to `develop`. (consistency with other workflows for the Yoast repos)
* Split step which was running multiple commands into two steps and combine a couple of those commands into a single command for improved error management. * Ensure all steps have a name. * Remove redundant comment.
... as, at this time, it isn't really needed.
Testing against PHPCS 4.x-dev has not been possible for a while and development of that version looks to be stagnated anyway, so testing against it is not really relevant at this time. This commit removes all references to testing against PHPCS 4.x from the workflow. If/when testing against the next PHPCS major becomes relevant again, this can brought back and adjusted based on the reality at that time.
Next iteration after PR 300. YoastCS 3.0 will not just have PHPCS and WPCS as dependencies for the sniff tests, but also PHPCSUtils. PR 300 already changed the logic of the workflow to take advantage of the Composer `--prefer-lowest` setting. This commit takes this yet another step further. **Stable vs Dev** As things were, the sniff tests were being run against the _lowest_ supported CS dependencies and the _development_ version of supported CS dependencies (with the exception of WPCS dev as WPCS 3.0.0 is not yet supported). While testing against the _dev_ versions of CS dependencies is relevant, it is more important that the tests safeguard that the sniffs work correctly on user-installable combinations of the CS dependencies. Now, it could be argued that every single combination of the different versions of each of these dependencies should be tested, but that would make the matrix _huge_ to little added benefit, especially as most of the time, the `lowest` and `stable` supported versions of CS dependencies will be the same for YoastCS (though not always, which still makes the distinction and running of tests against both `lowest` and `stable` relevant as it will allow for new releases of the CS dependencies automatically). To that end, I'm changing the test matrices to test by default against the `lowest` and `stable` supported versions of dependencies with some select extra builds testing against the `dev` versions of supported CS dependencies. PHP parse error linting will be run against all builds with the `stable` key now. Note: `stable` is also the name used for the Composer sister-argument to `--prefer-lowest`: `--prefer-stable`, so should be clear-cut for anyone reading the workflow. **Consequences** Quicktest workflow: * This will no longer contain exact versions of the CS dependencies in the test matrix, which should simplify workflow maintenance. * The "Composer update" step for the `dev` dependencies has been adjusted. Instead of multiple commands, it now does all the updates in one combined command, which should make debugging of the workflow easier. * One extra build has been added to test against `dev` dependencies. Test workflow: * The standard builds will now run against `lowest` and `stable` versions of dependencies and a number of extra builds have been added to test against `dev` versions of dependencies. I considered marking those as "allowed to fail" (`continue-on-error`), but decided against that as the sniffs should always be ready for the next minor/patch release of the CS dependencies. * The commented out build against WPCS dev can now be removed as that will be handled via the "all CS dependencies" `dev` builds (once WPCS 3.0 is supported and re-enabled for update for the `dev` builds). * The "Composer update" step for the `dev` dependencies has been adjusted. Instead of multiple commands, it now does all the updates in one combined command, which should make debugging of the workflow easier. * The "Composer update" step for the `lowest` depencies has been adjusted to be more readable. * Note that the direct Composer commands do not include `--update-with-dependencies` as our and our child dependencies overlap anyway. * The code coverage builds will now run against `lowest`, and `dev`, which is basically the same as it was before. Also note that the direct Composer command for updating the CS dependencies currently contain the `--ignore-platform-req=php+` argument. This is needed for PHP 8.x due to the max supported PHPUnit version being PHPUnit 7.x. This CLI argument can be removed once upstream PR squizlabs/PHP_CodeSniffer 3803 has been merged.
jrfnl
force-pushed
the
JRF/ghactions-update-workflows
branch
from
November 3, 2023 22:05
8b070a2
to
6131d41
Compare
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.
GH Actions: don't run CS checks on main
... as the can't be fixed on
main
anyway. This would need a PR todevelop
.(consistency with other workflows for the Yoast repos)
GH Actions/basics: tweak the workflow
GH Actions/test: remove the experimental key from the matrix
... as, at this time, it isn't really needed.
GH Actions/test: remove unused steps for PHPCS 4.x
Testing against PHPCS 4.x-dev has not been possible for a while and development of that version looks to be stagnated anyway, so testing against it is not really relevant at this time.
This commit removes all references to testing against PHPCS 4.x from the workflow.
If/when testing against the next PHPCS major becomes relevant again, this can brought back and adjusted based on the reality at that time.
GH Actions: tweak the way the PHPCS/WPCS versions are set
Next iteration after PR #300.
YoastCS 3.0 will not just have PHPCS and WPCS as dependencies for the sniff tests, but also PHPCSUtils.
PR #300 already changed the logic of the workflow to take advantage of the Composer
--prefer-lowest
setting. This commit takes this yet another step further.Stable vs Dev
As things were, the sniff tests were being run against the lowest supported CS dependencies and the development version of supported CS dependencies (with the exception of WPCS dev as WPCS 3.0.0 is not yet supported).
While testing against the dev versions of CS dependencies is relevant, it is more important that the tests safeguard that the sniffs work correctly on user-installable combinations of the CS dependencies.
Now, it could be argued that every single combination of the different versions of each of these dependencies should be tested, but that would make the matrix huge to little added benefit, especially as most of the time, the
lowest
andstable
supported versions of CS dependencies will be the same for YoastCS (though not always, which still makes the distinction and running of tests against bothlowest
andstable
relevant as it will allow for new releases of the CS dependencies automatically).To that end, I'm changing the test matrices to test by default against the
lowest
andstable
supported versions of dependencies with some select extra builds testing against thedev
versions of supported CS dependencies.PHP parse error linting will be run against all builds with the
stable
key now.Note:
stable
is also the name used for the Composer sister-argument to--prefer-lowest
:--prefer-stable
, so should be clear-cut for anyone reading the workflow.Consequences
Quicktest workflow:
dev
dependencies has been adjusted. Instead of multiple commands, it now does all the updates in one combined command, which should make debugging of the workflow easier.dev
dependencies.Test workflow:
lowest
andstable
versions of dependencies and a number of extra builds have been added to test againstdev
versions of dependencies.I considered marking those as "allowed to fail" (
continue-on-error
), but decided against that as the sniffs should always be ready for the next minor/patch release of the CS dependencies.dev
builds (once WPCS 3.0 is supported and re-enabled for update for thedev
builds).dev
dependencies has been adjusted. Instead of multiple commands, it now does all the updates in one combined command, which should make debugging of the workflow easier.lowest
depencies has been adjusted to be more readable.--update-with-dependencies
as our and our child dependencies overlap anyway.lowest
, anddev
, which is basically the same as it was before.Also note that the direct Composer command for updating the CS dependencies currently contain the
--ignore-platform-req=php+
argument.This is needed for PHP 8.x due to the max supported PHPUnit version being PHPUnit 7.x.
This CLI argument can be removed once upstream PR squizlabs/PHP_CodeSniffer#3803 has been merged.