Skip to content

Conversation

localheinz
Copy link
Contributor

This PR

  • runs the lint target using GitHub Actions

@@ -0,0 +1,40 @@
# https://help.github.com/en/categories/automating-your-workflow-with-github-actions

name: "Continuous Integration"
Copy link
Contributor Author

@localheinz localheinz Feb 4, 2020

Choose a reason for hiding this comment

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

If you prefer a different name for the workflow, let me know.

The name will be reflected on the badge.

For an example, see https://github.com/ergebnis/composer-normalize:

Screen Shot 2020-02-04 at 17 11 27

@@ -0,0 +1,40 @@
# https://help.github.com/en/categories/automating-your-workflow-with-github-actions

name: "Continuous Integration"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you prefer not to quote strings in YAML files, let me know, then I will adjust.

name: "Continuous Integration"

on:
pull_request:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will run a build for a pull request (not two).

pull_request:
push:
branches:
- "master"
Copy link
Contributor Author

@localheinz localheinz Feb 4, 2020

Choose a reason for hiding this comment

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

This will run a build for pushes into master (e.g., when a pull request is merged or a commit is pushed directly into master).

The Travis behavior can be reflected using

diff --git a/.github/continuous-integration.yml b/.github/continuous-integration.yml
index e0d82dec..743573a3 100644
--- a/.github/continuous-integration.yml
+++ b/.github/continuous-integration.yml
@@ -3,10 +3,8 @@
 name: "Continuous Integration"

 on:
-  pull_request:
-  push:
-    branches:
-      - "master"
+  - "pull_request"
+  - "push"

 jobs:
   lint:

strategy:
matrix:
php-binary:
- "php7.4"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since I'm not sure whether we need any specific extensions not already installed, perhaps it's sufficient not to use shivammathur/setup-php yet.


steps:
- name: "Checkout"
uses: "actions/checkout@v2.0.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See https://github.com/actions/checkout.

Note: There currently is no proper version resolving, so I believe it's best to reference tags.

Some action authors continually update the tag - for an example, see https://github.com/shivammathur/setup-php/releases.

For reference, see https://github.community/t5/GitHub-Actions/Version-numbering-for-Actions/m-p/32282#M1070.

- name: "Validate composer.json and composer.lock"
run: "composer validate --strict"

- name: "Cache dependencies installed with composer"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can remove this step if you prefer not to use caching.

run: "composer validate --strict"

- name: "Cache dependencies installed with composer"
uses: "actions/cache@v1.0.3"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@localheinz localheinz marked this pull request as ready for review February 4, 2020 16:10
@ondrejmirtes ondrejmirtes force-pushed the feature/github-actions branch from 4cc2368 to 887626a Compare February 4, 2020 21:17
@ondrejmirtes
Copy link
Member

I've pushed some commits. I currently don't know anything about GitHub Actions, but some things seem weird to me:

  • The yml file you added isn't in the workflows subfolder. (This is probably why the actions aren't run as part of this PR's build?)
  • There isn't the "setup PHP" step that seems to be in every PHP repo that uses GitHub actions.

Also, with this commit (887626a), the result will probably be different than I intend. I want each step to run independently of each other, in parallel. I suspect the structure would have to resemble something like this more (https://github.com/ergebnis/composer-normalize/blob/master/.github/workflows/continuous-integration.yml) to achieve that. But I'll wait for your answers to above items :)

Next steps after we sort out the things above:

  • Windows builds
  • Getting the PHPCS and PHPStan failures as part of the Checks tab here would be yummy :) I think it can somehow be achieved as part of the checkstyle format.
  • Removing equivalent actions running as part of GitHub Actions from .travis.yml.

@ondrejmirtes
Copy link
Member

Yeah, and we should run the checks on all PHP versions from 7.1 to 7.4 :)

@localheinz
Copy link
Contributor Author

localheinz commented Feb 4, 2020

I've pushed some commits. I currently don't know anything about GitHub Actions, but some things seem weird to me:

The yml file you added isn't in the workflows subfolder.

Yes, I fixed it.

(This is probably why the actions aren't run as part of this PR's build?)

The problem here is that the GitHub Actions aren't triggered unless

  • a workflow definition already exists in the target branch
  • a workflow is added by someone with push permissions

That is, if you push into this branch - now that I have moved the workflow definition to the correct directory - we should be able to see that workflow triggered.

There isn't the "setup PHP" step that seems to be in every PHP repo that uses GitHub actions.

I have added it now.

I had left it out intentionally because

  • composer.json does not explicitly define any extensions (which we could easily install and with shivammathur/setup-php)
  • we currently do not run builds against versions of PHP that are currently not available by default (e.g., PHP 7.0 or PHP 8.0)
  • the action adds some overhead

It has been added now!

Also, with this commit (887626a), the result will probably be different than I intend. I want each step to run independently of each other, in parallel. I suspect the structure would have to resemble something like this more (https://github.com/ergebnis/composer-normalize/blob/master/.github/workflows/continuous-integration.yml) to achieve that. But I'll wait for your answers to above items :)

I have split out the single lint job with multiple steps into multiple jobs that should run in parallel now (see comment above, actions will not be triggered by me because I do not have any push rights - if you add a commit here now, we should see workflows triggered).

Let's see how it goes!

👍

Next steps after we sort out the things above:

Windows builds
Getting the PHPCS and PHPStan failures as part of the Checks tab here would be yummy :) I think it can somehow be achieved as part of the checkstyle format.

Let's take a look as soon as the build has passed here, then we can either add https://github.com/staabm/annotate-pull-request-from-checkstyle or directly use cs2pr (has been added with shivammathur/setup-php#173)!

/cc @staabm

Removing equivalent actions running as part of GitHub Actions from .travis.yml.

Sounds good!

- "7.1"
- "7.2"
- "7.3"
- "7.4"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Question is: do we need run this

  • against all PHP versions
  • only the lowest supported one

?

I believe the latter suffices.

@localheinz localheinz force-pushed the feature/github-actions branch from b126968 to 9b2b6b7 Compare February 4, 2020 22:24
@@ -0,0 +1,187 @@
# https://help.github.com/en/categories/automating-your-workflow-with-github-actions

name: "Build"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The name used here does not need to match the name of the workflow definition file.

However, I think it makes sense when they have at least a similar name.

@ondrejmirtes ondrejmirtes force-pushed the feature/github-actions branch from 915f3f5 to cf29ebd Compare February 4, 2020 22:29
@ondrejmirtes
Copy link
Member

Awesome job so far, I like what I'm seeing :) I pushed some more commits into this branch and it doesn't look like Actions was triggered? I can push some minimal file to master so that we can get this running.

BTW I thought that setup-php was required, otherwise I didn't see how GitHub can figure out that we want to use PHP. But I think it will still be useful for Windows build.

One more question - some DRY in the definition file wouldn't hurt, do you think it's possible?

@ondrejmirtes ondrejmirtes force-pushed the feature/github-actions branch from cf29ebd to 6998ab5 Compare February 4, 2020 22:35
@ondrejmirtes
Copy link
Member

This has been bugging me sometimes:

Run composer update --no-interaction --no-progress --no-suggest
Loading composer repositories with package information
Updating dependencies (including require-dev)
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - phpstan/phpstan-strict-rules 0.12.0 requires phpstan/phpstan ^0.12 -> satisfiable by phpstan/phpstan[0.12.0, 0.12.1, 0.12.2, 0.12.3, 0.12.4, 0.12.5, 0.12.6, 0.12.7, 0.12.8, 0.12.9, 0.12.x-dev].
    - phpstan/phpstan-strict-rules 0.12.1 requires phpstan/phpstan ^0.12 -> satisfiable by phpstan/phpstan[0.12.0, 0.12.1, 0.12.2, 0.12.3, 0.12.4, 0.12.5, 0.12.6, 0.12.7, 0.12.8, 0.12.9, 0.12.x-dev].
    - phpstan/phpstan-strict-rules 0.12.2 requires phpstan/phpstan ^0.12.6 -> satisfiable by phpstan/phpstan[0.12.6, 0.12.7, 0.12.8, 0.12.9, 0.12.x-dev].
    - phpstan/phpstan-strict-rules 0.12.x-dev requires phpstan/phpstan ^0.12.6 -> satisfiable by phpstan/phpstan[0.12.6, 0.12.7, 0.12.8, 0.12.9, 0.12.x-dev].
    - don't install phpstan/phpstan 0.12.0|remove phpstan/phpstan-src dev-ff8fbbd7c1c48208057ffde818dc71c57ef263ae
    - don't install phpstan/phpstan 0.12.1|remove phpstan/phpstan-src dev-ff8fbbd7c1c48208057ffde818dc71c57ef263ae
    - don't install phpstan/phpstan 0.12.2|remove phpstan/phpstan-src dev-ff8fbbd7c1c48208057ffde818dc71c57ef263ae
    - don't install phpstan/phpstan 0.12.3|remove phpstan/phpstan-src dev-ff8fbbd7c1c48208057ffde818dc71c57ef263ae
    - don't install phpstan/phpstan 0.12.4|remove phpstan/phpstan-src dev-ff8fbbd7c1c48208057ffde818dc71c57ef263ae
    - don't install phpstan/phpstan 0.12.5|remove phpstan/phpstan-src dev-ff8fbbd7c1c48208057ffde818dc71c57ef263ae
    - don't install phpstan/phpstan 0.12.6|remove phpstan/phpstan-src dev-ff8fbbd7c1c48208057ffde818dc71c57ef263ae
    - don't install phpstan/phpstan 0.12.7|remove phpstan/phpstan-src dev-ff8fbbd7c1c48208057ffde818dc71c57ef263ae
    - don't install phpstan/phpstan 0.12.8|remove phpstan/phpstan-src dev-ff8fbbd7c1c48208057ffde818dc71c57ef263ae
    - don't install phpstan/phpstan 0.12.9|remove phpstan/phpstan-src dev-ff8fbbd7c1c48208057ffde818dc71c57ef263ae
    - don't install phpstan/phpstan 0.12.x-dev|remove phpstan/phpstan-src dev-ff8fbbd7c1c48208057ffde818dc71c57ef263ae
    - Installation request for phpstan/phpstan-src dev-ff8fbbd7c1c48208057ffde818dc71c57ef263ae -> satisfiable by phpstan/phpstan-src[dev-ff8fbbd7c1c48208057ffde818dc71c57ef263ae].
    - Installation request for phpstan/phpstan-strict-rules ^0.12 -> satisfiable by phpstan/phpstan-strict-rules[0.12.0, 

Not sure why it's not happening in pull requests on Travis...

@ondrejmirtes
Copy link
Member

I submitted the issue in Composer because I really don't know what to do about it: composer/composer#8579

@ondrejmirtes ondrejmirtes force-pushed the feature/github-actions branch from fee27da to ec62f69 Compare February 5, 2020 06:43
@staabm
Copy link
Contributor

staabm commented Feb 5, 2020

not sure why the build errors. maybe it is worth disabling the actions cache for now - it might affect the build.

@ondrejmirtes
Copy link
Member

The cache cannot be the reason for the failures since there's no job yet that succeded and populated the cache.

- "master"

env:
COMPOSER_ROOT_VERSION: "0.12.x-dev"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like this could be a potential workaround, @ondrejmirtes - I have moved it to the workflow level (so we don't have to repeat it).

It's currently not possible to use anchors - see https://github.community/t5/GitHub-Actions/Support-for-YAML-anchors/m-p/30336.

@ondrejmirtes ondrejmirtes force-pushed the feature/github-actions branch from e700f0b to 10d041a Compare February 6, 2020 12:29
@ondrejmirtes ondrejmirtes merged commit 10d041a into phpstan:master Feb 6, 2020
@localheinz localheinz deleted the feature/github-actions branch February 6, 2020 12:34
@localheinz
Copy link
Contributor Author

Thank you, @ondrejmirtes!

@ondrejmirtes
Copy link
Member

Thank you, it's perfect! I enjoyed this collaboration. Would you have time to look at the Windows build? My idea is:

  • Run tests on all PHPs from 7.1 to 7.4, but allow failures. Some stuff in Windows tests are different and I'm yet to come up with an elegant solution I'd like.
  • Run PHPStan on all PHPs from 7.1 to 7.4 and do not allow failure.

Then we could move onto phpstan/phpstan and convert all the current build checks to GitHub Actions as well. That will be easy.

And then we could do the hard stuff with secrets at the end, but it's not pressing. We already have most of the benefits GitHub Actions offer which is great.

What are the things with secrets:

  • Build and sign PHAR - needs a private GPG key that's currently encrypted for Travis at build/key.gpg.enc. We could publish the PHAR and the asc file as an artifact :)
    • There's a bit different workflow if we're just building a commit in master, or if we are building a tag. It's obvious from the Travis file.
  • Push the PHAR to phpstan/phpstan - needs my GitHub token for the permissions to clone and push to the repo.
  • Draft a release and upload phpstan.phar and phpstan.phar.asc as assets to it (when building a tag in phpstan/phpstan).
  • Update the playground - needs my AWS keys. (Currently turned off because of some issue with PHPStan 0.12.9).

@localheinz
Copy link
Contributor Author

@ondrejmirtes

Sure, let's continue working on this!

👍

@ondrejmirtes
Copy link
Member

Are we able to define dependencies between workflows? E.g. run whole phar.yml file only if build.yml succeeds?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants