- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Groom GitHub CI and move some checks from TravisCI to GitHub CI #5305
Conversation
@@ -54,6 +57,7 @@ jobs: | |||
uses: shivammathur/setup-php@v2 | |||
with: | |||
php-version: ${{ matrix.php-version }} | |||
coverage: none |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- name: Configure Symfony Flex | ||
run: | | ||
composer config extra.symfony.require ${{ matrix.execute-flex-with-symfony-version }} | ||
if: "matrix.execute-flex-with-symfony-version" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think if if:
would go before run:
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No preference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty solid to me.
@@ -78,13 +100,18 @@ jobs: | |||
retry_wait_seconds: 30 | |||
command: | | |||
composer update --optimize-autoloader --no-interaction --no-progress ${{ matrix.composer-flags }} | |||
composer info -D | sort |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is redundant with output of composer update
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find -D
and sort
useful
actually even more useful than output of update
. maybe let's use update -q
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would leave us with less information than actually provided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so... maybe let's keep both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's merge as-is and we can always adjust later ;)
5ca4728
to
4829a36
Compare
No description provided.