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

CI: switch to GitHub Actions #633

Merged
merged 2 commits into from
Mar 4, 2021
Merged

Conversation

jrfnl
Copy link
Collaborator

@jrfnl jrfnl commented Mar 3, 2021

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:

  • The Travis "sniff" and "lint" stages are combined in one GH Actions workflow job.
  • Any violations reported by the xmllint check as well as by the PHPCS check, will now be shown in-line in a PR.
  • The 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).
  • The build against PHP 8.1/nightly is commented out for the time being. See the description for the second commit for more details.

Fixes #628

Some ideas for potential future iterations

  • (Re-)enable a test run against PHP 8.1/nightly.
  • Add a "Quick test" script to run the tests against a more limited matrix for pushes only.
  • Implement use of the PHP Parallel Lint package for linting the PHP code and showing any errors in-line in a PR.
  • Implement use of the PHPCS feature completeness check.
  • Add a test run against PHPCS 4.x when it is closer to being released.
  • Register with coveralls.io (or another service) and start recording and checking code coverage for builds.

Commit details

CI: switch to GitHub Actions - step 1: sniff and lint stage

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.

CI: switch to GitHub Actions - step 2: test stage

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: PHP 8.1: fix deprecation notice squizlabs/PHP_CodeSniffer#3250

@jrfnl jrfnl added this to the 2.3.0 milestone Mar 3, 2021
@jrfnl jrfnl requested a review from a team as a code owner March 3, 2021 16:13
@jrfnl
Copy link
Collaborator Author

jrfnl commented Mar 3, 2021

Note: for this PR to become mergable, the "required statuses" will need to be updated.

GaryJones
GaryJones previously approved these changes Mar 4, 2021
Copy link
Contributor

@GaryJones GaryJones left a 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.

.github/workflows/basics.yml Outdated Show resolved Hide resolved
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
jobs:
checkcs:
name: 'Basic CS and QA checks'
runs-on: ubuntu-latest
Copy link
Contributor

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?

Copy link
Collaborator Author

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 ?

Copy link
Contributor

@GaryJones GaryJones left a comment

Choose a reason for hiding this comment

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

🎉 ✅

@GaryJones GaryJones merged commit 998c726 into develop Mar 4, 2021
@GaryJones GaryJones deleted the fix/628-switch-ci-to-gh-actions branch March 4, 2021 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI: move away from Travis-ci.org
2 participants