Yoast / duplicate-post

Yoast Duplicate Post plugin for WordPress
https://yoast.com
GNU General Public License v2.0
46 stars 35 forks source link

CI: switch to GitHub Actions #226

Closed jrfnl closed 2 years ago

jrfnl commented 2 years ago

Context

Summary

This PR can be summarized in the following changelog entry:

Relevant technical choices:

👉🏻 This PR will be easiest to review by reviewing each commit individually.

👉🏻 This PR (ab)uses the feature/ branch prefix on purpose to test the deploy workflow. Once this PR has been reviewed and merged, the feature branch for this PR should be removed from the dist repo.

.gitattributes: update

This updates the .gitattributes files to:

CI: switch to GitHub Actions - step 1: code style

This commit:

Notes:

  1. Builds will run on all pushes and on pull requests, except pushes to master.
  2. Builds can also be manually triggered. Note: manual triggering of builds has to be explicitly allowed. This is not a feature which is enabled by default.
  3. 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.
  4. Composer dependency downloads will be cached for faster builds using a predefined GH action 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
  5. The CS check will display the results in the actions script output log, as well as display any violations found inline in the GitHub code view using the cs2pr tool.

    Differences with the Travis implementation:

    • This check will now run on ALL pushes and pulls (except pushes to master). The branch filtering which was previously being applied in Travis has not been implemented for this script.
    • The composer validate command will now only be run against the PHP version used in the cs script (PHP 7.4), not against multiple PHP versions.

CI: switch to GitHub Actions - step 2: add security check

As this repo does not have a committed composer.lock file, we can just require the roave/security-advisories repository to handle the security check.

The CS, Lint and Test workflows all run a composer install, which will fail if with that package in place if it would be attempted to install an insecure dependency.

CI: switch to GitHub Actions - step 3: linting

This commit:

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

Differences with the Travis implementation:

CI: switch to GitHub Actions - step 4: PHP Unit tests

This commit:

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. 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=-1anddisplay_errors=Onto ensure **all** PHP notices are shown. Note: the defaults will be changed in the next major ofsetup-phpto be based on thephp.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 PHPUnit, I've elected not to use that option as we need to do a composer install anyway to get the other dependencies, like WP Test Utils.
  5. Composer dependency downloads will be cached for faster builds using a predefined GH action 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. As the Duplicate Post plugin only contains BrainMonkey based unit tests, we can (and should) use the most appropriate PHPUnit/BrainMonkey/Mockery/etc version for the PHP version the tests are being run on. However, as there is no committed composer.lock file and the composer.json file does not contain a platform - php configuration, just running a plain composer install will already take care of that. If, at some point in the future, the composer.lock file would be committed or a platform - php config would be added to the composer.json file, this will need revisiting.

Differences with the Travis implementation:

CI: switch to GitHub Actions - step 5: Deploy to dist

This commit:

Notes:

  1. Builds will run on:
    • Select pushes using branch filtering similar to before. Note: this branch has been set up to match one of the filters to allow for testing the deploy.
    • Pushed tags.
    • When manually triggered. Note: manual triggering of builds has to be explicitly allowed. 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 workflow global env key contains information about the Yoast dist repo. This information is used as "variables" in select places in the rest of the script. Important: while the default branch for the development repo may be different, the DIST_DEFAULT_BRANCH env variable should reflect the default branch for the dist repo. Also note that the script presumes that the sister-repo in the Yoast-dist organisation will be called the same as the repo in the Yoast organisation. If ever this would not be the case, an additional env variable could be set here and used later on in the script to handle that.
  4. The deployment is split into two jobs:
    • A prepare job which prepares the artifacts to deploy on the dev repo and uploads it as a workflow artifact.
    • A deploy job which checks out the dist version of the repo, cleans out most existing files, downloads and unpacks the prepared artifact and then commits the changes before pushing the branch/tag to the dist repo. The artifact upload/download mechanism with two jobs is used for two reasons:
      1. The way the environment in GH Actions is set up, is not all too friendly to checking out two repos at the same time and then being able to commit to the correct repo. (in other words: it's more secure) By splitting the deployment into two steps and uploading the artifact in between, we can be sure the commit ends up in the correct repo.
      2. To allow for examining the files in the artifacts for debugging.
  5. Both jobs are explicitly prevented from being run on forks outside the Yoast organisation.
  6. Both jobs contain a number of steps containing debug information, just in case.
  7. In various places in both jobs, data from the original commit is used, such as the branch name, the committer and the commit message. This type of data should always be regarded as untrusted input and when these github.... variables are used directly within the run context, they can lead to script injection and unintended execution of commands. To mitigate the risk of these type of script injection attacks, untrusted data is first set as a step-specific interim environment variable and only after that the environment variable (not the github variables directly) is used in the run context. This complies with the current best practices regarding defending against these type of attacks as per January 2022. A warning to this effect is at the top of the workflow to try and prevent people trying to be clever and "simplifying" the script. For more information, see: https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions
  8. The prepare job is explicitly set up to use PHP 5.6 for the Composer installs and Node 14 for yarn and grunt.
    • PHP 5.6 is used as that's the minimum supported PHP version for this repo and there is nothing preventing a successfull install on PHP 5.6.
    • Node 14 is used for consistency with other scripts which really need Node 14.
  9. The prepare job will do an explicit composer install ahead of creating the artifact. The grunt jobs actually run composer install multiple times, both with --no-dev and with --dev. By running the composer install ahead of the grunt job, we can use the ramsey/composer-install action to cache the downloads Composer needs, thus warming up the cache and speeding up the build.
  10. In the prepare job, the update of the version number in the source code will only be executed for tags, not for manually triggered deploys as that would lead to weird/undesired versioning.
  11. In the deploy job, the first two steps use some bash logic to create a short version of the commit sha and determine the branch which needs to be checked out and the commit title. While this type of logic, if simple, can oftentimes be used inline, it is often more descriptive to extract it to a separate step and this also prevents repetitive logic in multiple places. More complex logic will, in most cases, needs such an interim step setting a variable anyway.
  12. In the deploy job, a GitHub access token for the dist repo is used, which has been saved to the dev repos "secrets". This access token has been created with full access to the organisation and can commit to the dist repo. The use of GH access tokens and secrets this way is an advantage of using GH Actions and removes the need for creating an SSH key based on the config/travis/deploy_keys/id_rsa_yoast_dist.enc file as was previously done in Travis.
  13. In the deploy job, the commands which were previously executed in the config/travis/deploy_to_dist.sh script have been transformed into job steps.
  14. The commit for the deploy will be made by the "user" who created the tag.
  15. In contrast to Travis, the commit message, will no longer be the commit message of the commit which was pushed. Instead, the commit message will contain a range of information, including the commit message of the commit which was pushed, to be able to identify the commit and branch which triggered the deploy correctly.

Differences with the Travis implementation:

Test instructions

Test instructions for the acceptance test before the PR gets merged

This PR can be acceptance tested by following these steps:

These workflows have been extensively tested already and need no further testing.

Aside from the builds run for this PR, you can find the results of various specific tests also on the "Actions" page. For each job, it has been verified that the build(s) will actually show as failed when code has been altered to induce a fail condition.

diedexx commented 2 years ago

I've deleted the feature/switch-to-ghactions branch from yoast-dist, as requested in the PR description