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

Replace Travis CI with GitHub Actions workflows #447

Closed

Conversation

schlessera
Copy link
Member

No description provided.

@schlessera schlessera changed the base branch from master to add/phpunit-polyfills-library February 9, 2021 12:24
- name: Check out source code
uses: actions/checkout@v2

- name: Set up PHP envirnoment
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- name: Set up PHP envirnoment
- name: Set up PHP environment

(here and everywhere else)

@jrfnl
Copy link
Member

jrfnl commented Feb 9, 2021

Had a quick look through what is here so far (this is not a full review).

  • You may want to have a look at the ramsey/composer-install action - this will save on a lot of duplication with the composer install/composer caching.
  • IMO the lint action should be run - at least - on the lowest + highest supported PHP version.
    For a library like this which is used as a dependency for other projects, I'd even go so far as to run it on all supported PHP versions just to be sure.
  • Pro-tip: for the lint action, exclude the .git directory.
    This is not excluded by default in Parallel Lint and for "old" projects, not excluding it can really slow the action down a lot as the script will search the whole .git directory for PHP files.
    Oh and for this repo, you may also want to exclude the docs directory, though that shouldn't make much difference.
  • Suggestion: add a lint script to the composer.json file to make it easy for people to run the linting locally with the correct parameters:
    "lint": [
      "@php ./vendor/php-parallel-lint/php-parallel-lint/parallel-lint . -e php --exclude vendor --exclude .git
    ],
    Note:
    • the @php is to make sure the same PHP version is used as used by Composer (instead of a default PHP version on a system);
    • the full path name is for improved compatibility cross-platform (i.e. make sure it also works on Windows).
  • I hadn't come across the cs2pr usage before, but if it does what I expect it to do, I like it 👍
    Will have to nick that trick and apply it to various workflows in other projects 😉
  • I generally prefer to run the QA checks against all pushes, not just master, though for the tests - depending on how complex the matrix is - I sometimes use a simpler, only high/low job for pushes.
    Providing people check the action outcome before opening a PR, it should prevent PRs being opened which don't pass on the basics, which then have to get a second commit/force push to fix the PR as soon as it's opened.
  • You can add PHP 8.1 to the matrix for the unit job to test against the current PHP nightly.
    Use it in combination with continue-on-error to prevent it from failing/stopping the workflow run.
  • Once the script is finished, you may want to remove the fail-fast: false to be kind to the GH resources.

@jrfnl
Copy link
Member

jrfnl commented Feb 11, 2021

Just making a note here that this PR would close #420 + #439

@jrfnl
Copy link
Member

jrfnl commented Feb 11, 2021

Oh and another reminder: let's not forget to update the badges in the Readme once this PR is ready.

@jrfnl jrfnl modified the milestones: 2.0.0, 1.8.0 Mar 19, 2021
@jrfnl jrfnl mentioned this pull request Mar 20, 2021
14 tasks
@jrfnl jrfnl modified the milestones: 1.8.0, 2.0.0 Mar 20, 2021
@jrfnl
Copy link
Member

jrfnl commented Mar 20, 2021

I'm moving this back to the 2.0.0 milestone as we do still seem to be able to view the Travis runs, which allows us to release 1.8.0 knowing that CI is passing, so we can leave this until after the 1.8.0 release.

@schlessera
Copy link
Member Author

Closing this for now, as it is being superseded by #467

@schlessera schlessera closed this Apr 16, 2021
@schlessera schlessera deleted the add/github-actions-testing branch June 4, 2021 11:13
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