-
Notifications
You must be signed in to change notification settings - Fork 14
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
ISSUE-370: composer configuration checks #717
base: main
Are you sure you want to change the base?
ISSUE-370: composer configuration checks #717
Conversation
51e4e12
to
531adbd
Compare
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 approve this feature!
ddev composer config --unset extra.patches-file | ||
check_composer_install_contents "Store Composer patches configuration in \`composer.json\`" 0 | ||
|
||
ddev composer config extra.patches --json '{"drupal":{"issue-x":"http"}}' |
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 a great addition that aligns with our ADR: Use local copies of patch files
I know we have completely updated all our projects to adhere to this, so having a check that will enforce this would be a nice addition.
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.
Very cool!
Could we have this on by default and allow an opt-out?
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 great! I love codifying our ADRs like this 👏 .
I wonder if it's worth moving this to a new plugin class. The other code in this class is installing scaffold files, while this is validating composer which feels like a different concern.
src/ScaffoldInstallerPlugin.php
Outdated
* | ||
* @return void | ||
*/ | ||
private function _checkComposerPatchesAreLocal() |
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 we need to prefix these with an underscore - they're private methods?
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 got a VSCode hint when I started moving the code to the Drainpipe repo branch, and I thought it was a coding standard in the Drainpipe project. It is probably just a misconfiguration on my IDE. I will remove the _
from the checks I created.
Co-authored-by: Andrew Berry <andrew.berry@lullabot.com>
Opt-out using composer configuration in the "extra" section? Do you mean something like this? |
I think the answer is yes. Definitely. |
…hes-configuration' of github.com:Lullabot/drainpipe into issue-370-add-validation-and-warnings-for-composer-patches-configuration
If tests pass, the only thing missing is the opt-out configuration. #717 (review) |
…dd-validation-and-warnings-for-composer-patches-configuration
I also added documentation for the opt-out configurations. |
Added tests for the opt-out options. |
@beto-aveiga - @deviantintegral and I discussed this and we think it'd be really cool to have as a separate project as it doesn't necessarily need Drupal. We can then pull it into Drainpipe as a requirement. We've created a repo for you here and invited you: https://github.com/Lullabot/composer-checks Thanks! |
relates to: #370
It adds the following checks when running
composer install
.https://architecture.lullabot.com/adr/20220429-composer-patchlevel/
https://architecture.lullabot.com/adr/20220429-composer-patches-inline/
https://architecture.lullabot.com/adr/20220429-composer-patch-files/
https://architecture.lullabot.com/adr/20220429-composer-exit-failure/
It also adds a test for the checks.
Screenshots