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.

Sign up for GitHub

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

Upgrade GitHub actions #17

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3cbeaae
Upgrade GitHub actions
szepeviktor Oct 24, 2023
9019583
composer should validate what we have in the repo
szepeviktor Oct 24, 2023
e4c82a1
Restore master
szepeviktor Oct 24, 2023
200574c
Remove concurrency
szepeviktor Oct 24, 2023
7791317
Remove null
szepeviktor Oct 24, 2023
cd8288c
Re-add coverage
szepeviktor Oct 24, 2023
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
69 changes: 35 additions & 34 deletions .github/workflows/ci.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,45 +1,46 @@
# 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 •
edited
Loading

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
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 •
edited
Loading

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 •
edited
Loading

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!

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

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 +53,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 }}
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.

composer update --prefer-dist --no-interaction --no-progress

- 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.

Further investigation in https://github.com/szepeviktor/phpunit-vs-windows-for-sanmai


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 +102,35 @@ jobs:
- name: Upload coverage report
continue-on-error: false
env:
COVERALLS_REPO_TOKEN: ${{ secrets.GITHUB_TOKEN }}
COVERALLS_REPO_TOKEN: ${{ github.token }}
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.

run: |
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
2 changes: 1 addition & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ ci-cs: prerequisites
test: phpunit analyze composer-validate

.PHONY: composer-validate
composer-validate: test-prerequisites
composer-validate:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Composer should validate what we have in the repo, not generated files

Copy link
Owner

Choose a reason for hiding this comment

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

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.

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 we do composer update the lock file will always be okay!

Copy link
Owner

@sanmai sanmai Oct 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

$(SILENT) $(COMPOSER) validate --strict

test-prerequisites: prerequisites composer.lock
Expand Down