Move phpcs from Travis to GitHub Actions #3979

bendavies · 2020-04-28T20:06:14Z

Q	A
Type	improvement
BC Break	no
Fixed issues

Summary

Moves phpcs from Travis to GitHub Actions.

#3978 needs merging first.

.github/workflows/continuous-integration.yml

greg0ire · 2020-04-29T21:40:02Z

#3978 was merged :)

phpcs.xml.dist

.github/workflows/continuous-integration.yml

greg0ire

Please add cs2pr, I noticed @beberlei did that on ORM (master branch). I'm doing so in a PR to 2.7: doctrine/orm#8116

… with other jobs

.github/workflows/continuous-integration.yml

greg0ire

👌

morozov · 2020-05-05T22:39:57Z

Is there a way to see how cs2pr works in this repository before merging the changes? E.g. push a temporary commit that would violate the coding standard.

morozov · 2020-05-05T22:42:33Z

.github/workflows/continuous-integration.yml

@@ -6,28 +6,66 @@ on:

 jobs:
  static-analysis-psalm:
-    name: Static Analysis with Psalm
-    runs-on: ubuntu-latest
+    name: "Static Analysis with Psalm"


Are changes like this really needed? I'd rather keep it simple and only add quotes where required (e.g. strings with special characters, numeric strings that must remain strings, etc.).

This file will be maintained by humans who will not see the need to add quotes everywhere.

It is trying to maintain consistency across repos
https://github.com/doctrine/orm/pull/8116/files

So what's the reason why we want to have all strings quoted in all repositories?

i suspect inspiration from https://github.com/ergebnis/php-library-template/blob/master/.github/workflows/integrate.yaml

it's just coding style, and that repo enforces it:
https://github.com/ergebnis/php-library-template/blob/master/.yamllint.yaml

I personally don't like everything quoted, as you point out it's not necessary all the time. but being consistent is the main thing i guess.

cc @localheinz

@morozov

I am using ibiqlik/action-yamllint to lint YAML files using yamllint, and for consistency, I quote YAML strings.

As @bendavies pointed out, it's a matter of taste and consistency. I have to admit that I dislike having to escape a bunch of characters when quoting.

morozov · 2020-05-05T22:44:56Z

.github/workflows/continuous-integration.yml

        with:
          path: "~/.composer/cache"
-          key: "composer-${{ hashFiles('composer.lock') }}"
-          restore-keys: "composer-"
+          key: "php-${{ matrix.php-version }}-composer-locked-${{ hashFiles('composer.lock') }}"


This looks like an unnecessary complication. What's the scenario where we're going to run the code style check using different PHP versions?

This has already been discussed. It is trying to maintain consistency across repos
https://github.com/doctrine/orm/pull/8116/files

Could you point me to the thread, please? I could find the discussion regarding the build matrix.

doctrine/orm#8116 (comment)

sorry, let me elaborate, because that link doesn't help you.

I think, even if the matrix has a single php version, it enables us to use the same key everywhere, which helps cache hits. rather than using

key: "php-${{ matrix.php-version }}-composer-locked-${{ hashFiles('composer.lock') }}"

where a matrix is used and

key: "php-7.4-composer-locked-${{ hashFiles('composer.lock') }}"

where a matrix is not used.

So, by “everywhere” you mean that if named consistently, the same cache entry can be shared between multiple actions? Makes sense in this case.

Multiple action in the same repo yes. Thanks for phrasing it better!

morozov · 2020-05-05T22:45:53Z

.github/workflows/continuous-integration.yml

+    runs-on: "ubuntu-latest"
+
+    strategy:
+      matrix:


I believe the whole point of static analysis is that it's static and doesn't depend on the runtime environment. Having a matrix here looks like overkill to me.

It is trying to maintain consistency across repos
https://github.com/doctrine/orm/pull/8116/files

morozov · 2020-05-06T16:57:41Z

Looks good. Should we test #3979 (comment) and squash everything before the merge?

bendavies · 2020-05-06T18:08:57Z

Sure. I can do it later, or feel free to push to my branch if you are available now.

greg0ire · 2020-05-06T19:15:59Z

Looks like it works!

greg0ire · 2020-05-06T19:17:54Z

After merging up all the way until master, let's add this check as well as Psalm as required, shall we?

greg0ire · 2020-05-06T19:33:13Z

Thanks a lot @bendavies !

bendavies force-pushed the phpcs-github-actions branch 2 times, most recently from bb366de to 05be601 Compare April 28, 2020 20:08

bendavies changed the title ~~Phpcs GitHub actions~~ Move phpcs from Travis to GitHub Actions Apr 28, 2020

greg0ire reviewed Apr 28, 2020

View reviewed changes

.github/workflows/continuous-integration.yml Outdated Show resolved Hide resolved

greg0ire reviewed Apr 28, 2020

View reviewed changes

.github/workflows/continuous-integration.yml Show resolved Hide resolved

greg0ire reviewed Apr 28, 2020

View reviewed changes

.github/workflows/continuous-integration.yml Outdated Show resolved Hide resolved

bendavies force-pushed the phpcs-github-actions branch 3 times, most recently from fedae68 to 62a7028 Compare April 28, 2020 20:50

bendavies force-pushed the phpcs-github-actions branch from 62a7028 to c36f2aa Compare April 29, 2020 21:50

bendavies marked this pull request as ready for review April 29, 2020 21:51

bendavies requested a review from greg0ire April 29, 2020 21:51

carusogabriel reviewed Apr 30, 2020

View reviewed changes

phpcs.xml.dist Outdated Show resolved Hide resolved

bendavies force-pushed the phpcs-github-actions branch from c36f2aa to 3331b6d Compare April 30, 2020 11:59

greg0ire approved these changes May 1, 2020

View reviewed changes

greg0ire requested a review from morozov May 1, 2020 13:24

morozov reviewed May 1, 2020

View reviewed changes

.github/workflows/continuous-integration.yml Outdated Show resolved Hide resolved

greg0ire requested a review from carusogabriel May 1, 2020 19:22

greg0ire requested changes May 2, 2020

View reviewed changes

greg0ire mentioned this pull request May 2, 2020

Static analysis with Psalm doctrine/orm#8116

Merged

bendavies added 4 commits May 5, 2020 21:57

use consistent styling

3b6ab64

upgrade shivammathur/setup-php

258cbb3

use latest version of actions/cache

260d07f

use a build matrix for php version so cache keys can be easily shared…

bc9d8ca

… with other jobs

bendavies requested a review from morozov May 5, 2020 21:00

bendavies force-pushed the phpcs-github-actions branch from 3331b6d to 813ab87 Compare May 5, 2020 21:00

bendavies requested a review from greg0ire May 5, 2020 21:00

bendavies force-pushed the phpcs-github-actions branch 2 times, most recently from f1e446a to d034575 Compare May 5, 2020 21:02

bendavies force-pushed the phpcs-github-actions branch from d034575 to 25e00a9 Compare May 5, 2020 21:04

greg0ire reviewed May 5, 2020

View reviewed changes

.github/workflows/continuous-integration.yml Outdated Show resolved Hide resolved

.github/workflows/continuous-integration.yml Outdated Show resolved Hide resolved

move phpcs from travis to GitHub Actions

b46e86e

bendavies force-pushed the phpcs-github-actions branch from 25e00a9 to b46e86e Compare May 5, 2020 21:13

bendavies requested a review from greg0ire May 5, 2020 21:13

greg0ire previously approved these changes May 5, 2020

View reviewed changes

greg0ire added the CI label May 5, 2020

morozov reviewed May 5, 2020

View reviewed changes

greg0ire dismissed their stale review via 58bc4d4 May 6, 2020 19:12

greg0ire force-pushed the phpcs-github-actions branch from 58bc4d4 to b46e86e Compare May 6, 2020 19:16

greg0ire approved these changes May 6, 2020

View reviewed changes

greg0ire merged commit db5afa2 into doctrine:2.10.x May 6, 2020

greg0ire self-assigned this May 7, 2020

greg0ire added this to the 2.10.3 milestone Jun 7, 2020

github-actions bot locked as resolved and limited conversation to collaborators Aug 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move phpcs from Travis to GitHub Actions #3979

Move phpcs from Travis to GitHub Actions #3979

bendavies commented Apr 28, 2020

greg0ire commented Apr 29, 2020

greg0ire left a comment •

edited

Loading

greg0ire left a comment

morozov commented May 5, 2020

morozov May 5, 2020

bendavies May 6, 2020

morozov May 6, 2020

bendavies May 6, 2020

bendavies May 6, 2020

localheinz May 6, 2020

morozov May 5, 2020

bendavies May 6, 2020

morozov May 6, 2020

bendavies May 6, 2020

bendavies May 6, 2020 •

edited

Loading

morozov May 6, 2020

bendavies May 6, 2020

morozov May 5, 2020

bendavies May 6, 2020

morozov commented May 6, 2020

bendavies commented May 6, 2020

greg0ire commented May 6, 2020 •

edited

Loading

greg0ire commented May 6, 2020

greg0ire commented May 6, 2020

Move phpcs from Travis to GitHub Actions #3979

Move phpcs from Travis to GitHub Actions #3979

Conversation

bendavies commented Apr 28, 2020

Summary

greg0ire commented Apr 29, 2020

greg0ire left a comment • edited Loading

Choose a reason for hiding this comment

greg0ire left a comment

Choose a reason for hiding this comment

morozov commented May 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bendavies May 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morozov commented May 6, 2020

bendavies commented May 6, 2020

greg0ire commented May 6, 2020 • edited Loading

greg0ire commented May 6, 2020

greg0ire commented May 6, 2020

greg0ire left a comment •

edited

Loading

bendavies May 6, 2020 •

edited

Loading

greg0ire commented May 6, 2020 •

edited

Loading