Enhancement: Add workflow for GitHub Actions #153
Conversation
|
/cc @alcaeus |
localheinz
force-pushed
the
feature/github-actions
branch
3 times, most recently
from
809b3e4
to
b068f49
Compare
| - name: "Checkout" | ||
| uses: "actions/checkout@v2.0.0" | ||
|
|
||
| - name: "Build Docker image" |
There was a problem hiding this comment.
This step isn't actually necessary, but it makes it obvious that the Docker image needs to be build.
Otherwise, the build time will be attributed to the next step (where the Docker image is used).
| strategy: | ||
| matrix: | ||
| php-version: | ||
| - "7.2" |
There was a problem hiding this comment.
Not sure, do you think it is necessary to actually run phpcs on 7.3, 7.4, and 8.0?
There was a problem hiding this comment.
I'm not sure, but maybe when a sniff belongs to code only valid beginning with a certain PHP version.
There was a problem hiding this comment.
I believe the target PHP version should be expressed in the configuration of the standard in the project that implements it. See an example.
Static analysis should not produce different results with different runtimes, so more than one PHP version shouldn't be necessary.
There was a problem hiding this comment.
With previous discussion regarding version-dependent functionality (e.g. native type hints, see #144), testing gets quite complicated. The main build should test against the lowest supported version, which is 7.2.
However, our standard produces different results when running on different PHP versions. There is a mechanism with a patch file that allows for running on PHP 7.4 as well, but I feel that maintaining this mechanism for multiple versions will be quite cumbersome. However, changing this would be a policy change that should not be discussed in this PR.
So for now I'm fine running this only on 7.2. Before replacing our travis-ci builds with GitHub Actions, we should discuss the issue of running on separate versions separately.
localheinz
force-pushed
the
feature/github-actions
branch
from
b068f49
to
7eb8d3a
Compare
|
Thank you for contributing for the Github Action implementation. 馃憤 |
localheinz
force-pushed
the
feature/github-actions
branch
from
7eb8d3a
to
cf89a2a
Compare
| branches: | ||
| - "master" | ||
| schedule: | ||
| - cron: "0 17 * * *" |
There was a problem hiding this comment.
Looks like there's a cronjob scheduled to run once a day, doesn't it?
There was a problem hiding this comment.
Currently, the master branch is run every day. I'm fine running the master branch daily, but would prefer if we can also add the latest stable branch (7.0.x at the moment) every week.
There was a problem hiding this comment.
I believe that this is currently not possible, see https://help.github.com/en/actions/automating-your-workflow-with-github-actions/events-that-trigger-workflows#scheduled-events-schedule:
You can schedule a workflow to run at specific UTC times using POSIX cron syntax. Scheduled workflows run on the latest commit on the default or base branch. The shortest interval you can run scheduled workflows is once every 5 minutes.
There was a problem hiding this comment.
Ah, I see. That's fine for now :)
localheinz
force-pushed
the
feature/github-actions
branch
from
cf89a2a
to
46436a4
Compare
There was a problem hiding this comment.
Thanks for contributing this @localheinz! I'm in favour of getting this merged to see if GitHub Actions can replace travis-ci in the foreseeable future 馃帀
|
Thank you, @alcaeus! In case this doesn't work as expected once merged, I'll be available to quickly circle back and fix issues! |
|
@greg0ire since we currently don't require the tests to pass at all, I think it's ok to merge this and fix afterwards. |
|
Might be ok indeed, especially since that repo is not the most active, we will not confuse many contributors by doing so. |
|
I did the setup in case anyone wants to see: greg0ire#1 |
| - "7.2" | ||
| - "7.3" | ||
| - "7.4" | ||
| - "8.0" |
There was a problem hiding this comment.
This one fails for now but should be fixed by #156
localheinz
dismissed stale reviews from SenseException, greg0ire, and alcaeus
via
037e7ce
localheinz
force-pushed
the
feature/github-actions
branch
3 times, most recently
from
42ce35b
to
2913350
Compare
|
I have opened a pull request against my fork - perhaps a bit easier than having to update your branch: localheinz#1. |
localheinz
force-pushed
the
feature/github-actions
branch
from
2913350
to
9ab35ca
Compare
|
@alcaeus @greg0ire @morozov @SenseException @simPod Apart from the unrelated build failure (see #153 (comment)), the build passes now: |
|
@localheinz Shouldn't there be a status on your own repo? |
|
Thanks @localheinz! 馃帀 |
| uses: "actions/checkout@v2.0.0" | ||
|
|
||
| - name: "Install PHP" | ||
| uses: "shivammathur/setup-php@1.8.2" |
There was a problem hiding this comment.
i've said this before but i'm very much against this being used when docker containers are available.
docker makes it var easier for developers to replicate results locally.
There was a problem hiding this comment.
That's true, let's which direction we are taking here.
On the other hand, with Docker images we have the following issues:
- Docker images need to be mantained
- Docker images need to be built (either inline when running the job, which adds significant overhead, or built and tagged elsewhere, then downloaded)
I'm sure we'll be able to tweak things here and there!
There was a problem hiding this comment.
Hey!
Thanks for the reply.
I think I could say exactly the same about shivammathur/setup-php :)
- It is highly complex and maintenance is outside of our control here. We can either control Dockerfiles or use official php ones in most cases. I would trust official php docker images over
shivammathur/setup-php. shivammathur/setup-phphas significant overhead. It can take 25 seconds to install php, while pullingphp:7.4-cli-alpine3.11takes only 6 seconds for example, in a quick test.- As mentioned, i think the biggest thing is local reproducibility of build results.
Cheers!
There was a problem hiding this comment.
I am not sure one needs Docker to run this locally or? Its just php vendor/bin/phpcs, it doesn't have any dependencies or complexities to run.
|
Thank you, @alcaeus, @bendavies, @greg0ire, @malarzm, @morozov, @SenseException, and @simPod! |
|
@localheinz should we drop Travis ? Looks like you pretty much migrated everything to Github actions? |
|
馃憤 Drop it! |
|
See #220 :) |
This PR
馃拋鈥嶁檪 Not sure if this matches the expectations - I have tried to create a workflow with jobs equivalent to the stages currently used on Travis CI. Note that the actions will not be triggered here as I do not have push privileges (that's why I'm still leaving the Travis CI integration as it is).