Upgrade GitHub actions #17
Conversation
| @@ -1,45 +1,45 @@ | |||
| # yamllint disable rule:line-length | |||
| # yamllint disable rule:braces | |||
| # yaml-language-server: $schema=https://json.schemastore.org/github-workflow | |||
There was a problem hiding this comment.
This helps IDE-s to validate YAML while writing.
.github/workflows/ci.yaml
Outdated
|
|
||
| name: CI | ||
|
|
||
| on: | ||
| pull_request: | ||
| pull_request: null |
There was a problem hiding this comment.
This is the YAML default value. Strange!
There was a problem hiding this comment.
The docs never set the default value. I'd like to skip it here to because it is not an error, and it is confusing.
There was a problem hiding this comment.
I see what we have here in this PR.
You have no devops guy, and I do only devops.
There was a problem hiding this comment.
GitHub docs is for people who don't have 101 hours to fiddle with YAML and bytes in general. Who have to deliver today!
.github/workflows/ci.yaml
Outdated
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.ref }} | ||
| cancel-in-progress: true |
There was a problem hiding this comment.
Stop previous runs.
There was a problem hiding this comment.
I'd argue there's value in seeing previous builds finish to the end.
| runs-on: ubuntu-latest | ||
| strategy: | ||
| matrix: | ||
| php-version: ['7.4', '8.0', '8.1', '8.2', '8.3'] | ||
| coverage: ['pcov'] |
There was a problem hiding this comment.
A single value does not make a matrix.
There was a problem hiding this comment.
I reuse this workflow elsewhere, where I could use xdebug. And I don't want to maintain several vastly different workflows, please.
| @@ -52,38 +52,38 @@ jobs: | |||
| composer remove --no-update --dev --no-interaction --no-progress \ | |||
| phan/phan phpstan/phpstan vimeo/psalm \ | |||
| infection/infection friendsofphp/php-cs-fixer | |||
| composer update --prefer-dist --no-interaction --no-progress ${{ matrix.dependencies }} | |||
There was a problem hiding this comment.
Non-existent matrix value.
There was a problem hiding this comment.
- It is not an error.
- I have in another project, so if we make this change I won't be able to
git amchanges from here to there.
| php vendor/bin/phpunit | ||
| ./vendor/bin/phpunit |
There was a problem hiding this comment.
phpunit binary should be executable.
There was a problem hiding this comment.
It gets crazy in Windows, so I'd prefer the existing variant which, to my best knowledge, works everywhere.
There was a problem hiding this comment.
Further investigation in https://github.com/szepeviktor/phpunit-vs-windows-for-sanmai
| env: | ||
| PHP_VERSION: '8.2' | ||
| strategy: | ||
| matrix: | ||
| php-version: ['8.2'] |
There was a problem hiding this comment.
Always use a matrix.
There was a problem hiding this comment.
I'm confused: in one case you remove the matrix because "a single value does not make a matrix" and in another place you introduce the matrix with a single value 🤔
There was a problem hiding this comment.
Actually I would require tools in composer.json.
There was a problem hiding this comment.
Yeah, I think it actually runs PHP CS Fixer another time before the static analysis. The reason there's another stage for PHP CS Fixer because I want to be able to see that there are coding style issues right away, and last time I checked this was the fastest way to do just the code style check.
| COVERALLS_REPO_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| COVERALLS_REPO_TOKEN: ${{ github.token }} |
There was a problem hiding this comment.
The secret token has a new name.
| composer-validate: test-prerequisites | ||
| composer-validate: |
There was a problem hiding this comment.
Composer should validate what we have in the repo, not generated files
There was a problem hiding this comment.
This is exactly what I don't want because composer validate --strict will fail if the lock file is out of date. And we don't have a lock file unless we do composer install or like that.
There was a problem hiding this comment.
If we do composer update the lock file will always be okay!
There was a problem hiding this comment.
Exactly! But if we mess up the Makefile, and we don't do composer update because of a mistake, there's no way to see this! It is where composer validate comes to point that we messed up. Please put it back.
|
🔗 My take on checking PHP code |
| strategy: | ||
| matrix: | ||
| php-version: ['8.2'] | ||
| php-cs-fixer-version: ['v3.35.1'] |
There was a problem hiding this comment.
"A single value does not make a matrix." 😬
| @@ -36,10 +34,10 @@ jobs: | |||
|
|
|||
| - name: Validate composer.json | |||
| run: | | |||
| composer validate --strict | |||
| make composer-validate | |||
|
I am sorry. This is too hard for me. |
|
I'm sorry I made you feel bad. Would you mind if I tag you as a co-author for the changes I likes? |
Okay. |
Please see explanations below.