-
Notifications
You must be signed in to change notification settings - Fork 15
CI - fix not ussing "symfony-version" for composer package versions #55
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
Conversation
smhg
left a comment
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 think if you can try to basically keep composer.json unchanged, it looks promising!
Currently it isn't a great experience as things which should be CI-only now affect the dev experience for no good reason, but hopefully that isn't necessary (see code comments)?
| run: composer install --prefer-dist --no-interaction | ||
| run: | | ||
| composer global config --no-plugins allow-plugins.symfony/flex true | ||
| composer global require --no-progress --no-scripts --no-plugins symfony/flex |
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 think you can add --no-update here so the update happens in the next line only.
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 would still add this --no-update, because it speeds things up a bit I guess.
composer.json
Outdated
| "symfony/yaml": "^6.0||^7.0" | ||
| }, | ||
| "require-dev": { | ||
| "symfony/flex": "*", |
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 don't think this is necessary as it gets added in CI?
composer.json
Outdated
| }, | ||
| "extra": { | ||
| "symfony": { | ||
| "require": "7.0" |
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 seems to be handled by SYMFONY_REQUIRE in CI? I think you can remove it too?
composer.json
Outdated
| "scripts": { | ||
| "auto-scripts": { | ||
| "cache:clear": "symfony-cmd", | ||
| "assets:install %PUBLIC_DIR%": "symfony-cmd" |
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.
These scripts are only necessary for projects, not for libraries like PropelBundle, I think?
Done i made the changes you propose it look like everything still work right and each version is installed properly. |
| run: composer install --prefer-dist --no-interaction | ||
| run: | | ||
| composer global config --no-plugins allow-plugins.symfony/flex true | ||
| composer global require --no-progress --no-scripts --no-plugins symfony/flex |
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 would still add this --no-update, because it speeds things up a bit I guess.
|
I think it would be nice to fix these 2 last comments. But please yes, do merge it (--squash) into the 7.0 branch. It is a great improvement! Thank you! @SkyFoxvn |
|
i think the comment of the "exclude:" part is right on the spot it gives context for later reviews for use or for other ppl who think to fork the project about "--no-update" well its not working when you add it to the requir,e thats why i did remove it and i just uploaded it second time to confirm that so the result is ignores the given version of symfony components and install always the latest possible same as it was before |
No description provided.