Skip to content

Commit

Permalink
CI: switch to GitHub Actions - step 2: linting
Browse files Browse the repository at this point in the history
This commit:
* Adds a GH Actions workflow for the PHP lint CI check.
* Removes all references to that action from the `.travis.yml` configuration.
* Add a "Build Status" badge in the Readme to use the results from the GH `Lint` Action runs.

Notes:
1. Builds will run on:
    - Select pushes using branch filtering similar to before.
    - All pull requests.
    - When manually triggered.
        Note: manual triggering of builds has to be [explicitly allowed](https://github.blog/changelog/2020-07-06-github-actions-manual-triggers-with-workflow_dispatch/). This is not a feature which is enabled by default.
2. If a previous GH actions run for the same branch hadn't finished yet when the same branch is pushed again, the previous run will be cancelled.
    In Travis, this was an option on the "Settings" page - "Auto cancellation" -, which was turned on for most, if not all, Yoast repos. The `concurrency` configuration in the GHA script emulates the same behaviour.
3. The default ini settings used by the `setup-php` action are based on the `php.ini-production` configuration.
    This means that `error_reporting=E_ALL & ~E_DEPRECATED & ~E_STRICT`, `display_errors` is set to `Off` and `zend.assertions` is set to `-1` (= do not compile).
    For the purposes of CI, especially for linting and testing code, I'd recommend running with `zend.assertions=-1, `error_reporting=-1` and `display_errors=On` to ensure **all** PHP notices are shown.
    Note: the defaults will be changed in the next major of `setup-php` to be based on the `php.ini-develop` configuration, but that may still be a while off.
    Refs:
    * shivammathur/setup-php#450
    * shivammathur/setup-php#469
4. While the `setup-php` action offers the ability to [install the PHAR file for Parallel Lint](https://github.com/shivammathur/setup-php#wrench-tools-support), I've elected not to use that option as it would mean that we would not be able to use the `composer lint` script in the workflow, which would mean that the CLI arguments would have to be duplicated between the `composer.json` file and the `lint.yml` file.
    IMO, that would make this a typical point of failure where updates would be done in one, but not the other.
    If, at some point in the future, the Parallel Lint tool would start to support a config file for the CLI arguments, removing this point of failure, this choice can be (and should be) revisited.
5. Composer dependency downloads will be cached for faster builds using a [predefined GH action](https://github.com/marketplace/actions/install-composer-dependencies) specifically created for this purpose.
    The alternative would be to handle the caching manually, which would add three extra steps to the script.
    Note: Caching works differently between Travis and GH Actions.
    On GH Actions, once a cache has been created, it can't be updated. It can only be replaced by a new cache with a different key.
    As the PHP version, the `composer.json` and a potential `composer.lock` hash are all part of the key used by the above mentioned action, this difference should not have a significant impact.
    Ref: https://docs.github.com/en/actions/advanced-guides/caching-dependencies-to-speed-up-workflows
6. The Linting check will display the results inline in the GitHub code view using the [cs2pr](https://github.com/staabm/annotate-pull-request-from-checkstyle) tool.

Differences with the Travis implementation:
* There is a minor difference in the branch filtering being done for `push` events.
    Travis accepted PCRE regexes for the filtering. GH Actions uses glob patterns. The glob patterns now in place match the regexes as closely as possible, but are not an exact match to the "old" patterns. They should be sufficient for our purposes though.
* The tag-based branch filter has not been added as that filter was only intended for use with the `deploy` stage.
* Linting will now also be executed against PHP 8.1 and 8.2 (nightly), neither of which has yet been released.
    Linting runs against either of these two PHP versions will be "allowed to fail" for the time being.
    Note: if any of the "allowed to fail" jobs actually fail, the workflow will show as successfull, but there will still be a red `x` next to a PR.
    This is a known issue in GHA: https://github.com/actions/toolkit/issues/399
    There are work-arounds possible for this, however, the work-arounds would hide failures even more, meaning that chances are they won't be seen until they actually become a problem (no longer allowed to fail), which is too late.

:point_right: **Open question**: Should the prefixing be run during this workflow so the prefixed files will be scanned for parse errors as well ?
  • Loading branch information
jrfnl committed Oct 9, 2021
1 parent fe538d3 commit c0ab205
Show file tree
Hide file tree
Showing 3 changed files with 60 additions and 12 deletions.
55 changes: 55 additions & 0 deletions .github/workflows/lint.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
name: Lint

on:
# Run on pushes to select branches and on all pull requests.
push:
branches:
- master
- trunk
- 'release/**'
- 'hotfix/[0-9]+.[0-9]+*'
- 'feature/**'
pull_request:
# Allow manually triggering the workflow.
workflow_dispatch:

# Cancels all previous workflow runs for the same branch that have not yet completed.
concurrency:
# The concurrency group contains the workflow name and the branch name.
group: ${{ github.workflow }}-${{ github.head_ref }}
cancel-in-progress: true

jobs:
lint:
runs-on: ubuntu-latest

strategy:
matrix:
# Lint against the highest/lowest supported versions of each PHP major.
# And also do a run against "nightly" (the current dev version of PHP).
php_version: ['5.6', '7.0', '7.4', '8.0', '8.1', '8.2']

name: "Lint: PHP ${{ matrix.php_version }}"

# Allow builds to fail on as-of-yet unreleased PHP versions.
continue-on-error: ${{ matrix.php_version == '8.1' || matrix.php_version == '8.2' }}

steps:
- name: Checkout code
uses: actions/checkout@v2

- name: Install PHP
uses: shivammathur/setup-php@v2
with:
php-version: ${{ matrix.php_version }}
ini-values: zend.assertions=1, error_reporting=-1, display_errors=On
coverage: none
tools: cs2pr

# Install dependencies and handle caching in one go.
# @link https://github.com/marketplace/actions/install-composer-dependencies
- name: Install Composer dependencies
uses: ramsey/composer-install@v1

- name: Lint against parse errors
run: composer lint -- --checkstyle | cs2pr
16 changes: 4 additions & 12 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -39,17 +39,16 @@ jobs:
- php: 7.3.24
env: WP_VERSION=latest WP_MULTISITE=1 COVERAGE=1
- php: 5.6
env: WP_VERSION=5.6 WP_MULTISITE=1 PHPLINT=1 PHPUNIT=1
env: WP_VERSION=5.6 WP_MULTISITE=1 PHPUNIT=1
# Use 'trusty' to test against MySQL 5.6, 'xenial' contains 5.7 by default.
dist: trusty
- php: 7.3
env: WP_VERSION=master PHPUNIT=1
- php: 7.4
env: WP_VERSION=latest PHPUNIT=1 PHPLINT=1
env: WP_VERSION=latest PHPUNIT=1
- php: 8.0
env: WP_VERSION=latest PHPUNIT=1 PHPLINT=1
env: WP_VERSION=latest PHPUNIT=1
- php: "nightly"
env: PHPLINT=1
- stage: 🚀 deployment
name: "Deploy to S3"
if: branch = deploy # Only build when on the `deploy` branch, this functionality is not used yet and is taking a long time to complete.
Expand Down Expand Up @@ -153,7 +152,7 @@ install:
- |
if [[ ${TRAVIS_PHP_VERSION:0:1} == "8" || $TRAVIS_PHP_VERSION == "nightly" ]]; then
travis_retry composer install --no-interaction --ignore-platform-reqs --no-scripts --no-suggest
elif [[ "$PHPUNIT" == "1" || "$COVERAGE" == "1" || "$PHPLINT" == "1" ]]; then
elif [[ "$PHPUNIT" == "1" || "$COVERAGE" == "1" ]]; then
# Run composer update as we have dev dependencies locked at PHP ^7.0 versions.
travis_retry composer update --no-interaction --no-scripts
travis_retry composer install --no-interaction --no-scripts
Expand Down Expand Up @@ -236,13 +235,6 @@ script:
yarn test
travis_time_finish && travis_fold end "JavaScript.tests"
fi
# PHP Linting
- |
if [[ "$PHPLINT" == "1" ]]; then
travis_fold start "PHP.check" && travis_time_start
composer lint
travis_time_finish && travis_fold end "PHP.check"
fi
# PHP Unit
- |
if [[ "$PHPUNIT" == "1" && "$WP_VERSION" != "latest" ]] && [[ "$WP_VERSION" == "5.9" || "${WP_VERSION:0:1}" == "6" || "$WP_VERSION" == "master" ]]; then
Expand Down
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
# Yoast SEO

[![Lint](https://github.com/Yoast/wordpress-seo/actions/workflows/lint.yml/badge.svg)](https://github.com/Yoast/wordpress-seo/actions/workflows/lint.yml)
[![Build Status](https://api.travis-ci.com/Yoast/wordpress-seo.svg?branch=master)](https://travis-ci.com/Yoast/wordpress-seo)
[![Stable Version](https://poser.pugx.org/yoast/wordpress-seo/v/stable.svg)](https://packagist.org/packages/yoast/wordpress-seo)
[![License](https://poser.pugx.org/yoast/wordpress-seo/license.svg)](https://packagist.org/packages/yoast/wordpress-seo)
Expand Down

0 comments on commit c0ab205

Please sign in to comment.