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 GH Actions #137

Merged
merged 8 commits into from
Aug 16, 2021
Merged

CI: Switch to GH Actions #137

merged 8 commits into from
Aug 16, 2021

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Jun 13, 2021

Proposed Changes

In short: Travis is dead. Long live Travis! So, while a previous PR already moved a few parts of the Travis script to GH Actions, the remaining part of the Travis script should now also be converted to ensure that CI is run and builds are passing.

👉🏻 Note: the required statuses for the protected master branch need to be updated before this PR can be merged.

Commit details

Travis: remove steps already moved to GH Actions

Various CI steps were previously already moved to GH Actions via PR #96.

This removes those parts of the Travis script.

GH Actions/linting: allow for manually triggering a build

Triggering a workflow for a branch manually is not supported by default in GH Actions, but has to be explicitly allowed.

This is useful if, for instance, an external action script or composer dependency has broken.
Once a fix is available, failing builds for open PRs can be retriggered manually instead of having to be re-pushed to retrigger the workflow.

Ref: https://github.blog/changelog/2020-07-06-github-actions-manual-triggers-with-workflow_dispatch/

GH Actions: add phplint workflow

This commit:

  • Adds a dependency to the PHP Parallel Lint tool to handle the PHP linting.
    As of PHP Parallel Lint 1.3.0, the tool also supports PHP 5.3, making it a viable option for this repo.
    Ref: https://github.com/php-parallel-lint/PHP-Parallel-Lint/releases/tag/v1.3.0
  • Adds a Composer script to run Parallel Lint with preset optimized CLI arguments, making it easy for devs to run the tooling locally.
    Note: this uses @php path/to/tool to force the script to run under the same PHP version as Composer is run under in a cross-platform compatible manner.
  • Adds a separate GH Actions workflow to lint the code against the highest and lowest supported PHP versions, as well as two arbitrary interim PHP versions.
    This workflow has cs2pr enabled, which means that any issues found will be shown inline in PRs.
  • Removes the linting step from the Travis script.

GH Actions: add security-check workflow

  • Remove the enlightn/security-checker dependency which was added in Replace deprecated Sensiolabs security checker #130 as it conflicts with the minimum PHP requirements of this project (PHP 5.6 vs PHP 5.3).
    Also see Replace deprecated Sensiolabs security checker #131.
  • Adds a separate GH Actions workflow to check for insecure dependencies against the highest and lowest supported PHP versions using the fabpot/local-php-security-checker instead.
    Note: this workflow does a composer install as this package does not have a committed composer.lock file and we need the lock file to do the check on.
    Regarding the choice for this tool:
    • This tool will run independently of the PHP version used, making it more versatile.
    • It complies with most other criteria as set forth in Replace deprecated Sensiolabs security checker #131 (not deprecated, lightweight, versioned, easy to install/update).
    • As for running it locally - either devs can download the tool and run it locally or they can use the tooling available to run GH actions locally, so IMO that part is covered too.
  • Removes the security-check step from the Travis script.

Ref: https://github.com/fabpot/local-php-security-checker/releases/tag/v1.0.0

GH Actions: add integration test workflow

This commit:

  • Adds a separate GH Actions workflow to run the "integration tests" as previously run on Travis.
    The script is largely the same as the original Travis script with only some minor tweaks.
  • Removes the Travis script file as all actions have now been moved to GH Actions.

Notes:

  • This script is only run on pushes to master and on pull requests.
    For pushes to "feature branches" the next commit will add a slimmed down version of this script to conserve resources as this is a very extensive build matrix.

GH Actions: add a quicktest workflow

This commit:

  • Adds a separate GH Actions workflow to run the "integration tests" against a slimmed down matrix for pushes to "feature branches".
    The script part of this workflow is the same as the script in the integration test workflow.

README: switch Travis badge to GH Actions

I've chosen to only show a badge for the integration test workflow as that is the most important one after all.


Edit: I've added one extra commit.

🆕 GH Actions: set error reporting to E_ALL

Turns out the default setting for error_reporting used by the SetupPHP action is error_reporting=E_ALL & ~E_DEPRECATED & ~E_STRICT and display_errors is set to Off.

For the purposes of CI, I'd recommend running with E_ALL and display_errors=On to ensure all PHP notices are shown.

jrfnl added 4 commits June 13, 2021 14:07
Various CI steps were previously already moved to GH Actions via PR 96.

This removes those parts of the Travis script.
Triggering a workflow for a branch manually is not supported by default in GH Actions, but has to be explicitly allowed.

This is useful if, for instance, an external action script or composer dependency has broken.
Once a fix is available, failing builds for open PRs can be retriggered manually instead of having to be re-pushed to retrigger the workflow.

Ref: https://github.blog/changelog/2020-07-06-github-actions-manual-triggers-with-workflow_dispatch/
This commit:
* Adds a dependency to the PHP Parallel Lint tool to handle the PHP linting.
    As of PHP Parallel Lint 1.3.0, the tool also supports PHP 5.3, making a viable option.
    Ref: https://github.com/php-parallel-lint/PHP-Parallel-Lint/releases/tag/v1.3.0
* Adds a Composer script to run Parallel Lint with preset optimized CLI arguments, making it easy for devs to run the tooling locally.
    Note: this uses `@php path/to/tool` to force the script to run under the same PHP version as Composer is run under in a cross-platform compatible manner.
* Adds a separate GH Actions workflow to lint the code against the highest and lowest supported PHP versions, as well as two arbitrary interim PHP versions.
    This workflow has `cs2pr` enabled, which means that any issues found will be shown inline in PRs.
* Removes the linting step from the Travis script.
* Remove the `enlightn/security-checker` dependency which was added in 130 as it conflicts with the minimum PHP requirements of this project (PHP 5.6 vs PHP 5.3).
    Also see 131.
* Adds a separate GH Actions workflow to check for insecure dependencies against the highest and lowest supported PHP versions using the `fabpot/local-php-security-checker` instead.
    Note: this workflow does a `composer install` as this package does not have a committed `composer.lock` file and we need the `lock` file to do the check on.
    Regarding the choice for this tool:
    - This tool will run independently of the PHP version used, making it more versatile.
    - It complies with most other criteria as set forth in 131 (not deprecated, lightweight, versioned, easy to install/update).
    - As for running it locally - either devs can download the tool and run it locally or they can use the tooling available to run GH actions locally, so IMO that part is covered too.
* Removes the security-check step from the Travis script.

Ref: https://github.com/fabpot/local-php-security-checker/releases/tag/v1.0.0
jrfnl added 3 commits June 13, 2021 16:51
This commit:
* Adds a separate GH Actions workflow to run the "integration tests" as previously run on Travis.
    The script is largely the same as the original Travis script with only some minor tweaks.
* Removes the Travis script file as all actions have now been moved to GH Actions.

Notes:
* This script is only run on pushes to `master` and on pull requests.
    For pushes to "feature branches" the next commit will add a slimmed down version of this script to conserve resources as this is a very extensive build.
This commit:
* Adds a separate GH Actions workflow to run the "integration tests" against a slimmed down matrix for pushes to "feature branches".
    The script part of this workflow is the same as the script in the integration test workflow.
I've chosen to only show a badge for the integration test workflow as that is the most important one after all.
@jrfnl jrfnl force-pushed the feature/switch-to-ghactions branch 2 times, most recently from f64d204 to 52d2c44 Compare June 13, 2021 15:05
@jrfnl
Copy link
Member Author

jrfnl commented Jun 13, 2021

@Potherca I'd love your help on the yaml warnings. I'm always already happy when the yaml doesn't have parse errors, actually works and is readable. As for fixing the warnings displayed here: I honestly wouldn't know where to start. Yaml to me is an annoyance at best, a mystery language without specification understandable by humans at worst.

Note: I tried to use the multi-line yaml syntax as was previously used in Travis for some of the "line length too long" commands, but that doesn't seem to work with GH Actions as the commands then don't run or run incorrectly.

Example: https://github.com/Dealerdirect/phpcodesniffer-composer-installer/runs/2814150580?check_suite_focus=true

@jrfnl jrfnl requested a review from mjrider June 13, 2021 15:13
Potherca
Potherca previously approved these changes Jun 14, 2021
Turns out the default setting for `error_reporting` used by the SetupPHP action is `error_reporting=E_ALL & ~E_DEPRECATED & ~E_STRICT` and `display_errors` is set to `Off`.

For the purposes of CI, I'd recommend running with `E_ALL` and `display_errors=On` to ensure **all** PHP notices are shown.
@jrfnl
Copy link
Member Author

jrfnl commented Jun 21, 2021

FYI: I've added one extra commit based on information I've recently received.

@jrfnl
Copy link
Member Author

jrfnl commented Aug 11, 2021

@Potherca Could you have another look ? and can we get this merged ?
Would be good to get confirmation that the build still pass on PHP 8.1.

@Potherca Potherca enabled auto-merge August 16, 2021 14:00
@jrfnl
Copy link
Member Author

jrfnl commented Aug 16, 2021

@Potherca Auto-merge wouldn't work as the Travis builds were still required statuses, but as this PR is removing the Travis config, Travis would never report back (not that it was still reporting anyway).

I've removed Travis from the required statuses now, though we should add the integration test and PHP linting jobs to the required statuses, but as that has to be done one by one, I need to find some time for that....

@Potherca
Copy link
Member

Clicked the wrong button out of habit 🤦 Merging for real now!

@Potherca Potherca disabled auto-merge August 16, 2021 14:43
@Potherca Potherca merged commit 7d5cb88 into master Aug 16, 2021
@Potherca Potherca deleted the feature/switch-to-ghactions branch August 16, 2021 14:43
@jrfnl
Copy link
Member Author

jrfnl commented Aug 16, 2021

@Potherca FYI: I've updated the required statuses properly now.

All tasks in all workflows are set as required, with the exception of 1) PHPCS 4.x test runs (as still in dev) and 2) PHP 8.1.
Though I do think we can probably set PHP 8.1 as required as well, though PHPCS still throws a lot of notices (PRs to fix this upstream have already been pulled).

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.

2 participants