Trying to utilise GH actions for running checks instead of travis by dingo-d · Pull Request #261 · WPTT/WPThemeReview

dingo-d · 2020-11-28T19:26:58Z

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
- used to run the sniff stage where we run the basic CI checks (xml lint, composer checks, etc.) on the latest PHP 7 version (I think PHP 8 would fail, at least from what I tested locally)
test.yaml
- used to run unit tests on different PHP versions (ranging from 5.4 to 8.0)

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

act -j qa_checks -P ubuntu-latest=shivammathur/node:latest -s GITHUB_TOKEN=

act -j tests -P ubuntu-latest=shivammathur/node:latest -s GITHUB_TOKEN=

You'll need to add the GH token so that you avoid GitHub's rate limit for composer.

No need to clutter checks

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.

desrosj

Left some small feedback, but looks great overall!

desrosj · 2020-12-17T19:20:41Z

+    qa_checks:
+        name: Quality control checks
+        runs-on: ubuntu-latest
+        env:


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.

Good point. For the sniff stage, there is basically only one check, so I can just hardcode them.

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.

desrosj · 2020-12-17T19:24:19Z

+                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:


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.

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 😄

desrosj · 2020-12-17T19:24:30Z

+            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']


Suggested change

wpcs-versions: [ '2.3.0', 'dev-master', 'dev-develop']

wpcs-versions: [ '2.3.0', 'dev-master', 'dev-develop' ]

desrosj · 2020-12-17T19:26:45Z

+                PHPCS_BRANCH: ${{ matrix.phpcs-versions }}
+                WPCS_BRANCH: ${{ matrix.wpcs-versions }}
+              run: |
+                if [[ ${WPCS_BRANCH} == "dev-develop" ]]; then composer config minimum-stability dev; fi


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.

desrosj · 2020-12-17T19:27:27Z

+
+            - name: Check the code-style consistency of the xml files.
+              run: |
+                export XMLLINT_INDENT=$'\t'


Any reason not to just define this as a global environment variable instead?

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 👍🏼

desrosj · 2020-12-17T19:30:30Z

+
+on:
+    pull_request:
+        branches: [ master, develop ]


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.

desrosj · 2020-12-17T19:32:02Z

+
+jobs:
+    tests:
+        name: Unit tests on PHP ${{ matrix.php-versions }} with PHPCS ${{ matrix.phpcs-versions }} and WPCS ${{ matrix.wpcs-versions }}


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.

Good point. The name is cut out atm when checking the actions tab. Will change this 🙂

dingo-d · 2020-12-18T08:45:02Z

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.

desrosj · 2020-12-18T14:15:57Z

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).

dingo-d · 2020-12-20T10:33:27Z

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 😄

Potherca · 2020-12-20T12:16:34Z

@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}

jrfnl · 2020-12-20T12:32:48Z

@Potherca I like it 👍
One question: how would this work if you need to run multiple xmllint commands ?
Would they all need to be in the same step ? Would you need to use the action multiple times ?

Example:
https://github.com/PHPCompatibility/PHPCompatibilityParagonie/blob/7a71578674453fc293bd8e7b201347a0663b6d16/.github/workflows/ci.yml#L35-L51

Potherca · 2020-12-20T13:39:39Z

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")

jrfnl · 2020-12-20T14:50:15Z

Either should work:

Would it not install xmllint twice then ? Or is a safeguard for that build in ?

Potherca · 2020-12-20T16:00:08Z

Nothing is really "installed", the xmllint provided by the docker image in pipeline-componets/xmllint action is (re)used.
since the action already lives on GitHub, the overhead of retrieving the action (or related docker image) is negligible.

dingo-d · 2020-12-21T08:40:16Z

I really like this idea. Makes the workflow file a lot cleaner 👍🏼

Potherca · 2020-12-21T10:39:23Z

I'll be creating the XML Lint Pipeline Component in the coming days, I'll leave a comment here when its done. 👍

Potherca · 2021-01-03T12:36:36Z

The XML Lint pipeline-component was completed, I am in the process of implementing it in sniff.yml, will post again as soon as I've got it working. 👍

... 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.

... based on latest example code generate on the Actions page.

Add the sniff stage

75e4059

dingo-d added the enhancement label Nov 28, 2020

dingo-d self-assigned this Nov 28, 2020

dingo-d added 7 commits November 28, 2020 20:32

Remove travis file

dcfe1ad

No need to clutter checks

Setup xml lint

dab3683

Remove XMLLINT_INDENT export

da02fd2

Add empty test.yaml file

6f0d414

Fix the diff?

4d595d3

Trying to fix the diff

6eaefd0

Try to run on xenial

8bfb9df

dingo-d mentioned this pull request Dec 9, 2020

Move from TravisCI to GitHub Actions WordPress/WordPress-Coding-Standards#1965

Merged

dingo-d added 9 commits December 12, 2020 11:47

Add lint check and composer validation to workflow

9d76af2

Trying to add test stage

90274d3

Try to exclude certain versions

e17ba7f

Only allow master phpcs and develop wpcs on one PHP version, and that should be allowed to fail

Refine exclusions

ccac4f8

We only need master - master and lowest - lowest for every PHP version, not all 4 possible combinations.

PHP exclusions cannot be in the list apparently

e8a8e9d

Adding php 5.4 and 5.5

dfc2177

Fix typo

0874eb7

Add nightly to build

024c796

Fix the sniff workflow

699bbd2

dingo-d requested a review from jrfnl December 13, 2020 10:51

desrosj suggested changes Dec 17, 2020

View reviewed changes

dingo-d added 6 commits December 20, 2020 09:38

Update suggestions for dealerdirect plugin

22215e1

So that it works with the composer v2

Remove travis reference

935a581

Fix the minor grammar issues in the Contributing file

f38c3ee

Also change the theme review team repo to correct repo name. It's themes team now.

Fix the sniff stage accordding to the PR suggestions

7e52ae9

Also, some ideas from PHPCompatibility PR PHPCompatibility/PHPCompatibility#1260 were implemented as well (composer dependencies and xml violations).

Update test workflow with the suggestions from the PR

ee596d4

Trying to fix the invalid yaml issue

12aa082

dingo-d added 2 commits December 20, 2020 11:06

Add continue-on-error directive

9c0697e

Fix the badges and the repo name

0797dd1

jrfnl added 10 commits February 19, 2021 18:00

GH Actions: remove the ignores for markdown files

d526f26

... as it would prevent PRs with only markdown changes from being merged due to required statuses.

GH Actions: allow for manually triggering a workflow

e87f9e9

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/

GH Actions: remove GH token from setup PHP step

e7e9def

... as it shouldn't be needed.

GH Actions: add some inline documentation for some steps

40d2f74

GH Actions: use Parallel-Lint for PHP linting

70f794f

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.

GH actions: set the PHP ini values

4177063

GH actions: remove duplication of setting of PHPCS/WPCS versions

924643e

GH actions: handle Composer install on PHP 8

032fe1b

jrfnl force-pushed the add-gh-actions branch from 66277bc to 032fe1b Compare February 19, 2021 18:33

jrfnl and others added 2 commits February 19, 2021 19:43

Readme: fix up the badges

abd53e0

... based on latest example code generate on the Actions page.

Remove extra quotes and reduced spacing from 4 to 2 spaces

6546201

jrfnl approved these changes Feb 19, 2021

View reviewed changes

jrfnl merged commit 44ad1b0 into develop Feb 19, 2021

jrfnl deleted the add-gh-actions branch February 19, 2021 19:43

This was referenced Feb 19, 2021

Add allow failures for WPCS dev-develop #259

Closed

Install phpcsutils for travis tests when testing with phpcs:dev-master #256

Closed

jrfnl mentioned this pull request Jun 22, 2021

README: minor fix #266

Merged

	wpcs-versions: [ '2.3.0', 'dev-master', 'dev-develop']
	wpcs-versions: [ '2.3.0', 'dev-master', 'dev-develop' ]

Uh oh!

Conversation

dingo-d commented Nov 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

EDIT

How to run this locally?

Uh oh!

desrosj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dingo-d commented Dec 18, 2020

Uh oh!

desrosj commented Dec 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dingo-d commented Dec 20, 2020

Uh oh!

Potherca commented Dec 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrfnl commented Dec 20, 2020

Uh oh!

Potherca commented Dec 20, 2020

Uh oh!

jrfnl commented Dec 20, 2020

Uh oh!

Potherca commented Dec 20, 2020

Uh oh!

dingo-d commented Dec 21, 2020

Uh oh!

Potherca commented Dec 21, 2020

Uh oh!

Potherca commented Jan 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dingo-d commented Nov 28, 2020 •

edited

Loading

desrosj commented Dec 18, 2020 •

edited

Loading

Potherca commented Dec 20, 2020 •

edited

Loading