-
Notifications
You must be signed in to change notification settings - Fork 149
Move the CI from Travis CI to GitHub Actions #208
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
Conversation
4a5f4fc to
2d15f9d
Compare
6276936 to
48071b3
Compare
|
Code coverage support will come in a second PR. |
ab28431 to
a3c87ef
Compare
a3c87ef to
6ef6468
Compare
sabberworm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot! I had wanted to switch to GH actions for some time now but lacked the time to work on it.
| # https://help.github.com/en/categories/automating-your-workflow-with-github-actions | ||
|
|
||
| on: | ||
| pull_request: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK
pull_request:
types: [opened, synchronize]
should suffice, i.e. we don’t need to re-run the build for other PR actions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can change this.
Out of curiosity: Are there any other actions? (Closing a PR does not trigger a build AFAIK.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are the following pull_request activity types:
- assigned
- unassigned
- labeled
- unlabeled
- opened
- edited
- closed
- reopened
- synchronize
- ready_for_review
- locked
- unlocked
- review_requested
- review_request_removed
But, what I wasn’t aware of, when you don’t specify types explicitly, only opened, synchronize and reopened trigger builds so we’d only get rid of reopened so I guess you can leave it as-is.
(Though I would assume reopened PRs still have their previous builds lying around so I don’t know if there’s a need to actually rebuild but since reopens are relatively rare, it shouldn’t matter).
|
|
||
| - name: Install Composer dependencies | ||
| run: | | ||
| composer update --with-dependencies --no-progress; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should always install from the lock file for CI builds.
| composer update --with-dependencies --no-progress; | |
| composer install --no-progress; |
There can be a separate action to update dependencies regularly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I've adapted those two changes. (I have also proposed a PR to get rid of composer.lock as only project should have this file, but not libraries: #209)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I hadn’t yet seen that PR, sorry. Is this recommendation documented anywhere? I would prefer to not break builds just because dependencies have released updated patch versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know whether this is documented somewhere. I learned this from a talk on Composer by Nils Adermann (one of the persons behind Packagist).
The basic idea is:
- Projects have a
composer.lockas they are expected to be deployed with exactly specified versions of their dependencies, and changes to the dependencies should be separate explicit changes. - Libraries are expected to work with all allowed versions of their dependencies. So they should have no
composer.lock, and CI should run the tests (at least the unit tests) both with the lowest as well as the highest versions of the dependencies. (This is something for which I will provide a PR once this is PR here merged.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this change has broken the build. I guess (without being able to spend time on this before later today) that we need different sets of dependency versions for different PHP versions.
I'll have a look at this later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has turned out that we need to keep using composer update. If we use composer install, the installation will fail if the composer.lock contains package versions that are not installable with the current PHP version: https://github.com/oliverklee/PHP-CSS-Parser/runs/2381731583?check_suite_focus=true
I'll undo this change in a minute, squash and repush.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. You can see the build status here on my fork: https://github.com/oliverklee/PHP-CSS-Parser/actions/runs/764969412
(In your repository, the builds will only start after the merge.)
Travis CI has dropped free support for open source projects. To continue having CI builds, we now use GitHub Actions for this.
136862a to
2ee292f
Compare
Travis CI has dropped free support for open source projects.
To continue having CI builds, we now use GitHub Actions for this.