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

Fixes #3796: Deploy command continues despite composer failure #3809

Merged
merged 5 commits into from
Aug 14, 2019

Conversation

nhmbounteous
Copy link
Contributor

Fixes #3796

Changes proposed

Added logic check to see if the composer install command was successful, and if not interrupt the deployment to prevent corrupt environment code

Steps to replicate the issue

  1. Create a semi-corrupt compsoer.json
  2. Example, apply incorrect patch to module
"patches": {
    "drupal/svg_image_field": {
      "This doesn't apply": "https://www.drupal.org/files/issues/entity-browser-view-context-2865928-14.patch"
    },
  1. Run blt artifact:deploy
  2. The deployment will display the error [Exception] Cannot apply patch This doesn't apply (https://www.drupal.org/files/issues/entity-browser-view-context-2865928-14.patch)!
  3. The error will be ignored and cause a broken artifact to be pushed to the environment.

Previous (bad) behavior, before applying PR

Corrupt environment code on deployment and ignored the composer error

Expected behavior, after applying PR and re-running test steps

An exception is now thrown and the process is stopped, preventing a corrupted artifact deployment

@nhmbounteous nhmbounteous changed the title Bugfix/issue 3796 Bugfix #3796: Deploy command continues despite composer failure Aug 13, 2019
Added space in between if statement for code standards
Updated composer error message
Copy link

@josephdpurcell josephdpurcell left a comment

Choose a reason for hiding this comment

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

I would like to see this handle other cases. This only handles the composer case.

However, handling one case is better than none, and it addresses exactly #3796. Approving.

Copy link
Contributor

@danepowell danepowell left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the contribution. The code looks good (following the pattern used elsewhere in BLT) and I've verified that it fixes the issue.

This awaits one more internal review.

We will fix this more generally with a refactor in #3811

@danepowell danepowell added the Bug Something isn't working label Aug 13, 2019
@danepowell danepowell changed the title Bugfix #3796: Deploy command continues despite composer failure Fixes #3796: Deploy command continues despite composer failure Aug 13, 2019
Copy link
Contributor

@alexxed alexxed left a comment

Choose a reason for hiding this comment

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

Looks fine, but I wonder if Robo has something to automatically throw in these cases.

@danepowell
Copy link
Contributor

@alexxed we are checking on that with the Robo maintainers here: consolidation/robo#891

But for now, it looks like no, Robo does not throw exceptions, it just provides an exit code.

@danepowell danepowell merged commit 0242392 into acquia:10.x Aug 14, 2019
danepowell pushed a commit that referenced this pull request Aug 15, 2019
* Made the deploy command fail and stop on `composer install` exception.

* Made the deploy command fail and stop on `composer install` exception.

* Update DeployCommand.php

Added space in between if statement for code standards

* Update DeployCommand.php

Updated composer error message
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DT-669: Deploy command continues despite composer failure
5 participants