PHPCSStandards / composer-installer

Composer installer for PHP_CodeSniffer coding standards
https://packagist.org/packages/dealerdirect/phpcodesniffer-composer-installer
MIT License
549 stars 36 forks source link

CI: Switch to GH Actions #137

Closed jrfnl closed 3 years ago

jrfnl commented 3 years ago

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:

GH Actions: add security-check workflow

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

GH Actions: add integration test workflow

This commit:

Notes:

GH Actions: add a quicktest workflow

This commit:

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 commented 3 years ago

@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 commented 3 years ago

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

jrfnl commented 3 years ago

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

jrfnl commented 3 years ago

@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 commented 3 years ago

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

jrfnl commented 3 years ago

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