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
Trying to utilise GH actions for running checks instead of travis #261
Conversation
Only allow master phpcs and develop wpcs on one PHP version, and that should be allowed to fail
We only need master - master and lowest - lowest for every PHP version, not all 4 possible combinations.
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.
Left some small feedback, but looks great overall!
.github/workflows/sniff.yaml
Outdated
qa_checks: | ||
name: Quality control checks | ||
runs-on: ubuntu-latest | ||
env: |
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.
I don't feel strongly about removing these, but because there is only one branch of PHPCS and WPCS used in this workflow, these environment variables could be removed entirely in favor of just hard-coding them below in the commands.
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.
Good point. For the sniff stage, there is basically only one check, so I can just hardcode them.
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.
Thinking this through the next day, the only argument for leaving them is consistency across workflows. If the same env
variables are actually needed in other workflow files, I could see leaving them in all workflows for consistency's sake.
.github/workflows/test.yaml
Outdated
php-versions: [ '5.4', '5.5', '5.6', '7.0', '7.1', '7.2', '7.3', '7.4', '8.0', '8.1' ] | ||
phpcs-versions: [ '3.5.0', 'dev-master' ] | ||
wpcs-versions: [ '2.3.0', 'dev-master', 'dev-develop'] | ||
exclude: |
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.
I struggled with what to do here in the WPCS workflow. I elected to exclude certain branches from the matrix in favor of using include
statements. The number of includes
that result was less than the number of excludes
combinations that way. Not sure if that would also be true here as well.
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.
Yes, noticed that in the WPCS PR and it definitely looks a lot cleaner. I'll refactor this. For the first pass, I tried having the same output from the Travis here in GH actions so this was a quick and dirty way of achieving this 😄
.github/workflows/test.yaml
Outdated
matrix: | ||
php-versions: [ '5.4', '5.5', '5.6', '7.0', '7.1', '7.2', '7.3', '7.4', '8.0', '8.1' ] | ||
phpcs-versions: [ '3.5.0', 'dev-master' ] | ||
wpcs-versions: [ '2.3.0', 'dev-master', 'dev-develop'] |
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.
wpcs-versions: [ '2.3.0', 'dev-master', 'dev-develop'] | |
wpcs-versions: [ '2.3.0', 'dev-master', 'dev-develop' ] |
.github/workflows/test.yaml
Outdated
PHPCS_BRANCH: ${{ matrix.phpcs-versions }} | ||
WPCS_BRANCH: ${{ matrix.wpcs-versions }} | ||
run: | | ||
if [[ ${WPCS_BRANCH} == "dev-develop" ]]; then composer config minimum-stability dev; fi |
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.
For clarity, let's split this out into it's own step with an if:
. An expression can be used to evaluate ${{ matrix.wpcs-versions == 'dev-develop' }}
.
In my opinion this makes it more clear what runs during the workflow.
.github/workflows/sniff.yaml
Outdated
|
||
- name: Check the code-style consistency of the xml files. | ||
run: | | ||
export XMLLINT_INDENT=$'\t' |
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.
Any reason not to just define this as a global environment variable instead?
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.
Just because I wasn't aware that was an option (saw it in your WPCS PR) 😅
I will definitely move it to a global env var 👍🏼
.github/workflows/test.yaml
Outdated
|
||
on: | ||
pull_request: | ||
branches: [ master, develop ] |
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.
Just documenting that this will work based on: https://docs.github.com/en/free-pro-team@latest/actions/reference/workflow-syntax-for-github-actions#onpushpull_requestbranchestags.
I was originally looking here and did not see the branches documented.
.github/workflows/test.yaml
Outdated
|
||
jobs: | ||
tests: | ||
name: Unit tests on PHP ${{ matrix.php-versions }} with PHPCS ${{ matrix.phpcs-versions }} and WPCS ${{ matrix.wpcs-versions }} |
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.
Let's make this less verbose. We know that we're running unit tests since this is the test workflow.
I'd say PHP X with PHPCS Y/WPCS Z
. This is what displays as the job title in the left hand sidebar, so if we can make it short, it will be easier to scan to find the exact job we want. Otherwise, the PHPCS and WPCS versions will be ticked away.
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.
Good point. The name is cut out atm when checking the actions tab. Will change this 🙂
Thanks for the suggestions @desrosj I really appreciate it. I'll refactor this so that it looks a lot more like your WPCS PR, with allowed failures and cleaner syntax. |
Any time! Great work here, especially in a first pass! I'll be a bit absent until the new year, but I'll check in when I notice changes as I'm able. |
So that it works with the composer v2
Also change the theme review team repo to correct repo name. It's themes team now.
Also, some ideas from PHPCompatibility PR PHPCompatibility/PHPCompatibility#1260 were implemented as well (composer dependencies and xml violations).
I've fixed the comments from the PR, and also made some additional changes so that it works on composer v2 (dealerdirect had to be raised to 0.7). Also, fixed some readme, contributing, and other minor issues (I could cherry-pick them to a separate PR to keep this one cleaner 🤔 ). @jrfnl if you have the time I'd love if you could check this out. It's a lot smaller and I recon easier to check than the WPCS one 😄 |
@dingo-d I'm thinking of creating an action (as part of @pipeline-components) for combining these: - name: Install xmllint
run: sudo apt-get install --no-install-recommends -y libxml2-utils
# Show XML violations inline in the file diff.
# @link https://github.com/marketplace/actions/xmllint-problem-matcher
- uses: korelstar/xmllint-problem-matcher@v1
- name: Validate ruleset against schema
run: xmllint --noout --schema ./vendor/squizlabs/php_codesniffer/phpcs.xsd ./*/ruleset.xml into this: - name: xmllint
uses: pipeline-componets/xmllint@v1
run: xmllint --noout --schema ./vendor/squizlabs/php_codesniffer/phpcs.xsd ./*/ruleset.xml Is that something you would be interested in? //CC: @mjrider |
@Potherca I like it 👍 |
Either should work: Run multiple commands in one step - uses: pipeline-componets/xmllint@v1
run: |
xmllint --noout --schema vendor/squizlabs/php_codesniffer/phpcs.xsd ./*/ruleset.xml
diff -B ./PHPCompatibilityParagonieRandomCompat/ruleset.xml <(xmllint --format "./PHPCompatibilityParagonieRandomCompat/ruleset.xml")
diff -B ./PHPCompatibilityParagonieSodiumCompat/ruleset.xml <(xmllint --format "./PHPCompatibilityParagonieSodiumCompat/ruleset.xml") Run both commands in a separate step - uses: pipeline-componets/xmllint@v1
run: xmllint --noout --schema vendor/squizlabs/php_codesniffer/phpcs.xsd ./*/ruleset.xml
- uses: pipeline-componets/xmllint@v1
run: |
diff -B ./PHPCompatibilityParagonieRandomCompat/ruleset.xml <(xmllint --format "./PHPCompatibilityParagonieRandomCompat/ruleset.xml")
diff -B ./PHPCompatibilityParagonieSodiumCompat/ruleset.xml <(xmllint --format "./PHPCompatibilityParagonieSodiumCompat/ruleset.xml") |
Would it not install xmllint twice then ? Or is a safeguard for that build in ? |
Nothing is really "installed", the |
I really like this idea. Makes the workflow file a lot cleaner 👍🏼 |
I'll be creating the XML Lint Pipeline Component in the coming days, I'll leave a comment here when its done. 👍 |
The XML Lint pipeline-component was completed, I am in the process of implementing it in |
... as it would prevent PRs with only markdown changes from being merged due to required statuses.
Triggering a workflow for a branch manually is not supported by default in GH Actions, but has to be explicitly allowed. Ref: https://github.blog/changelog/2020-07-06-github-actions-manual-triggers-with-workflow_dispatch/
... as it shouldn't be needed.
As things were, the code was only ever linted against PHP 7.4, while in the original Travis workflow, the code was linted against every single supported PHP version. This adds a hybrid workflow in which the code is linted against the high/low of each PHP major within the range of supported PHP versions.
Not a necessity, but allows for: 1. Faster linting due to the parallel option. 2. The ability to display linting errors in the PR via the cs2pr tooling.
The minimum supported PHPCS version according to the `composer.json` file, is `3.3.1`, so that should be the minimum version to test against. Similarly, the minimum supported WPCS version according to the `composer.json` file is `2.2.0`. For PHP 7.4 and 8.0, the builds have to be specified explicitly as PHP 7.4 only has PHPCS runtime support as of PHPCS 3.5.0 and PHP 8.0, as of PHPCS 3.5.7. As the matrix builds only test high PHPCS with high WPCS and low PHPCS with low WPCS, for PHP 7.2, in the Travis config, explicit builds were specified to reverse that combination, i.e. high PHPCS with low WPCS and low PHPCS with high WPCS. This has now been implemented here as well. Includes adding some inline documentation to the matrix. Note: I've not added a build against PHP 8.1/nightly as the PHPUnit version we are forced to use via PHPCS (PHPUnit 7.x) is not compatible with PHP 8.1, so the build wouldn't pass no matter what.
66277bc
to
032fe1b
Compare
... based on latest example code generate on the Actions page.
This is an attempt to run actions instead of travis and see if we can acchieve everything using them.
The travis stages will be split into 2 workflow files:
sniff.yaml
test.yaml
I'll see if this works and if we'll achieve what we have on travis currently. Plus we can see which one is faster if we manage to do this 😄
EDIT
How to run this locally?
If anybody is interested in running the workflows locally, you'll need to install act.
You can run the stages individually with
You'll need to add the GH token so that you avoid GitHub's rate limit for composer.