Skip to content
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

Upgrade GitHub actions #17

Closed
wants to merge 6 commits into from
Closed
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 35 additions & 35 deletions .github/workflows/ci.yaml
Original file line number Diff line number Diff line change
@@ -1,45 +1,45 @@
# yamllint disable rule:line-length
# yamllint disable rule:braces
# yaml-language-server: $schema=https://json.schemastore.org/github-workflow

Copy link
Contributor Author

@szepeviktor szepeviktor Oct 24, 2023

Choose a reason for hiding this comment

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

This helps IDE-s to validate YAML while writing.

name: CI

on:
pull_request:
pull_request: null
push:
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 is the YAML default value. Strange!

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@szepeviktor szepeviktor Oct 24, 2023

Choose a reason for hiding this comment

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

I see what we have here in this PR.
You have no devops guy, and I do only devops.

Copy link
Contributor Author

@szepeviktor szepeviktor Oct 24, 2023

Choose a reason for hiding this comment

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

The docs never set the default value.

GitHub docs is for people who don't have 101 hours to fiddle with YAML and bytes in general. Who have to deliver today!

branches:
- master
- main
szepeviktor marked this conversation as resolved.
Show resolved Hide resolved

concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Stop previous runs.

Copy link
Owner

Choose a reason for hiding this comment

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

I'd argue there's value in seeing previous builds finish to the end.

env:
COMPOSER_ROOT_VERSION: 0.1.x-dev
COMPOSER_ROOT_VERSION: '0.1.x-dev'

jobs:
tests:
name: Test with PHP ${{ matrix.php-version }}
runs-on: ubuntu-latest
strategy:
matrix:
php-version: ['7.4', '8.0', '8.1', '8.2', '8.3']
coverage: ['pcov']

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A single value does not make a matrix.

Copy link
Owner

Choose a reason for hiding this comment

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

I reuse this workflow elsewhere, where I could use xdebug. And I don't want to maintain several vastly different workflows, please.

name: Test with PHP ${{ matrix.php-version }}

steps:
- name: Checkout code
uses: actions/checkout@v2
- name: Checkout repository
uses: actions/checkout@v4

- name: Setup PHP
uses: shivammathur/setup-php@v2
with:
php-version: ${{ matrix.php-version }}
coverage: ${{ matrix.coverage }}
coverage: pcov
tools: composer:v2

- name: Validate composer.json
run: |
composer validate --strict
make composer-validate

Copy link
Owner

Choose a reason for hiding this comment

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

Please put it back as well 🙏

- name: Cache dependencies
uses: actions/cache@v2
uses: actions/cache@v3
with:
path: ~/.cache/composer
key: composer-${{ matrix.php-version }}-${{ hashFiles('composer.*') }}
Expand All @@ -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 }}
composer update --prefer-dist --no-interaction --no-progress
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Non-existent matrix value.

Copy link
Owner

Choose a reason for hiding this comment

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

  • It is not an error.
  • I have in another project, so if we make this change I won't be able to git am changes from here to there.


- name: Execute tests
run: |
php vendor/bin/phpunit
./vendor/bin/phpunit

Comment on lines -59 to +57
Copy link
Contributor Author

Choose a reason for hiding this comment

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

phpunit binary should be executable.

Copy link
Owner

Choose a reason for hiding this comment

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

It gets crazy in Windows, so I'd prefer the existing variant which, to my best knowledge, works everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

validate:
name: Static Analysis and Validation
runs-on: ubuntu-latest

env:
PHP_VERSION: '8.2'
strategy:
matrix:
php-version: ['8.2']

Comment on lines -65 to +64
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Always use a matrix.

Copy link
Owner

Choose a reason for hiding this comment

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

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 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I would require tools in composer.json.

Copy link
Owner

Choose a reason for hiding this comment

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

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.

steps:
- name: Checkout code
uses: actions/checkout@v2
- name: Checkout repository
uses: actions/checkout@v4

- name: Setup PHP
uses: shivammathur/setup-php@v2
with:
php-version: ${{ env.PHP_VERSION }}
php-version: ${{ matrix.php-version }}
extensions: ast
coverage: pcov
tools: composer:v2

- name: Cache dependencies
uses: actions/cache@v2
uses: actions/cache@v3
with:
path: ~/.cache/composer
key: composer-${{ env.PHP_VERSION }}-${{ hashFiles('composer.*') }}
key: composer-${{ matrix.php-version }}-${{ hashFiles('composer.*') }}
restore-keys: |
composer-${{ env.PHP_VERSION }}-
composer-${{ matrix.php-version }}-
composer-

- name: Install dependencies
Expand All @@ -101,35 +101,35 @@ jobs:
- name: Upload coverage report
continue-on-error: false
env:
COVERALLS_REPO_TOKEN: ${{ secrets.GITHUB_TOKEN }}
COVERALLS_REPO_TOKEN: ${{ github.token }}
run: |
Comment on lines -104 to +102
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 secret token has a new name.

php vendor/bin/php-coveralls -v
./vendor/bin/php-coveralls -v

coding-standards:
name: Coding Standards
runs-on: ubuntu-latest

env:
PHP_VERSION: '8.2'
PHP_CS_FIXER_VERSION: 'v3.35.1'
strategy:
matrix:
php-version: ['8.2']
php-cs-fixer-version: ['v3.35.1']

Copy link
Owner

Choose a reason for hiding this comment

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

"A single value does not make a matrix." 😬

steps:
- name: Checkout
uses: actions/checkout@v2
uses: actions/checkout@v4

- name: Setup PHP
uses: shivammathur/setup-php@v2
with:
php-version: ${{ env.PHP_VERSION }}
tools: php-cs-fixer:${{ env.PHP_CS_FIXER_VERSION }}
php-version: ${{ matrix.php-version }}
tools: php-cs-fixer:${{ matrix.php-cs-fixer-version }}

- name: Restore PHP-CS-Fixer cache
uses: actions/cache@v2
uses: actions/cache@v3
with:
path: .php_cs.cache
key: php-cs-fixer
restore-keys: php-cs-fixer

- name: Run PHP-CS-Fixer, version ${{ env.PHP_CS_FIXER_VERSION }}
- name: Run PHP-CS-Fixer ${{ matrix.php-cs-fixer-version }}
run: |
php-cs-fixer fix --diff --dry-run --verbose
Loading