-
Notifications
You must be signed in to change notification settings - Fork 40
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
CI: switch to GitHub Actions #633
Conversation
Note: for this PR to become mergable, the "required statuses" will need to be updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved with one small query.
This commit: * Adds a GH Actions workflow for the CI checks which were previously run on Travis in the `sniff` and `lint` stages. For the badge and workflow display, the name `BasicQA` seems an appropriately descriptive name. * Removes that part of the `.travis.yml` configuration. * Removes a duplicate line in the `bin/xml-lint` script used by this workflow. Notes: 1. In the Travis script, these checks were in two separate stages. I've now combined them into one workflow `job`. In GH Actions, the environment has to be setup as part of the workflow. And as the environment needed for these checks is largely the same, combining the checks into one workflow simplifies the script and reduced duplication. 2. Builds will run on all pushes and on pull requests, with the exception of pushes just modifying files which are irrelevant to this workflow. 3. The workflow can be manually (re-)triggered if needs be. 4. Any violations reported by the XML validation against the XSD schema, as well as coming from the PHPCS run, are set up to be shown in-line in the pull request to easily see what line in the pull request causes a build to fail.
This commit: * Adds a GH Actions workflow for the CI check which was previously run on Travis in the `test` stage. * Removes the, now redundant, `.travis.yml` configuration. * Updates the `.gitattributes` file. Notes: 1. For the time being, I've commented out the job against PHP 8.1/nightly as - no matter what I've tried - I can't even get the tests running at the moment, so they would only ever fail. Once PHP 8.1 starts to release alpha/beta/RC builds, further testing should be done to attempt to get this working. Notes/current findings: - With PHPUnit 7.x on PHP 8.1, using a configuration file will block the tests from running. - In some cases (PHPCS itself, PHPCSExtra), I managed to get the tests running by passing all relevant information via the command-line and actively ignoring the configuration file using `--no-configuration`. Unfortunately, that does not seem to work in this case. Further investigation is needed. - Based on the (running) tests for PHPCSExtra on PHP 8.1, an incompatibility with PHP 8.1 in PHPCS itself has already been discovered. A fix for this has been pulled, but not yet merged: squizlabs/php_codesniffer 3250
3255484
to
7b04b16
Compare
jobs: | ||
checkcs: | ||
name: 'Basic CS and QA checks' | ||
runs-on: ubuntu-latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests have got a warning:
Ubuntu-latest workflows will use Ubuntu-20.04 soon. For more details, see actions/runner-images#1816
Presumably this won't matter as there is shivammathur/setup-php@v2
being used for Basic and Test workflows already anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That warning is not directly related to the setup-php action. It is related to the jobs all using runs-on: ubuntu-latest
.
For all intends and purposes, the ubuntu version shouldn't have any significant effect on the PHP environment (though, yes, that is providing the setup-php
action will work on all), so I've been ignoring the warning.
Using ubuntu-latest
is a shorthand way to always use the latest instead of a "fixed" version.
With Travis, there was a lot of differences between the images (trusty
, xenial
, bionic
) and what they supported and it was more important to "fix" it to a specific version as it impacted which versions of PHP were available.
I don't have the impression the difference is as big on GH Actions, nor that it will impact the workflows we use.
Where it would, I'm expecting the setup-php
action to fill in the difference.
Also see: https://github.com/shivammathur/setup-php/#cloud-osplatform-support
If and when GH actually switches over and the build would fail, we could always (temporarily) fix it to a specific ubuntu
version until support for the next one has been repaired.
More than anything, it's about us not having to keep track of the specifics of the images and not having to update the name of the images used every so often and trusting that the actions we use will handle it.
Does that explain it well enough ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 ✅
TL;DR
This switches out the Travis script for GitHub Actions workflows.
This initial pull request is a one-on-one translation of the original CI script to GH actions.
The only differences are:
xmllint
check as well as by the PHPCS check, will now be shown in-line in a PR.test
job is set up to allow for supporting multiple WPCS versions, even though VIPCS at the time of writing only supports one version (2.3.0
).Fixes #628
Some ideas for potential future iterations
Commit details
CI: switch to GitHub Actions - step 1: sniff and lint stage
This commit:
sniff
andlint
stages.For the badge and workflow display, the name
BasicQA
seems an appropriately descriptive name..travis.yml
configuration.bin/xml-lint
script used by this workflow.Notes:
job
.In GH Actions, the environment has to be setup as part of the workflow. And as the environment needed for these checks is largely the same, combining the checks into one workflow simplifies the script and reduced duplication.
CI: switch to GitHub Actions - step 2: test stage
This commit:
test
stage..travis.yml
configuration..gitattributes
file.Notes:
Once PHP 8.1 starts to release alpha/beta/RC builds, further testing should be done to attempt to get this working.
Notes/current findings:
--no-configuration
. Unfortunately, that does not seem to work in this case. Further investigation is needed.A fix for this has been pulled, but not yet merged: PHP 8.1: fix deprecation notice squizlabs/PHP_CodeSniffer#3250