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

Fail Drush Deploy if Database Updates Produce a Diff Against Tracked Configuration (#5389) #5713

Closed
wants to merge 5 commits into from

Conversation

adamzimmermann
Copy link
Contributor

Motivation

Known Issues

  • I targeted this against 11.x as the site I'm working on daily won't let me get to 12.x due to issues I can't resolve at the moment. I'm happy to port this to 12.x once we agree on the general approach. This just helps me test it more easily.
  • Adding some better logging/visibility into the differences it detected would be great. I'm happy to add that too once we have an agreement on the general approach/messaging we want.
  • I'm unsure if the return is the preferred way to abort a process here, but figured someone with more knowledge could quickly point me in the right direction.

Current State

I tested this locally and it does work though. I added an update hook that simply did this and it failed as expected. ✅

/**
 * Force a failure in the drush deploy command.
 */
function example_update_8004() {
  \Drupal::service('module_installer')->uninstall(['some_module']);
}

Screenshot 2023-07-21 at 10 34 19 AM

@adamzimmermann adamzimmermann changed the title Update composer for Drush 12 (#5389) Fail Drush Deploy if Database Updates Produce a Diff Against Tracked Configuration (#5389) Jul 21, 2023
@weitzman weitzman changed the base branch from 11.x to 12.x July 21, 2023 16:04
@weitzman weitzman changed the base branch from 12.x to 11.x July 21, 2023 16:05
Copy link
Member

@weitzman weitzman left a comment

Choose a reason for hiding this comment

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

Nice work! Also needs to go into 12.x first, and test coverage for should be expanded for deploy - that can be a followup.

src/Commands/core/DeployCommands.php Outdated Show resolved Hide resolved
src/Commands/core/DeployCommands.php Outdated Show resolved Hide resolved
src/Commands/core/DeployCommands.php Outdated Show resolved Hide resolved
src/Commands/core/DeployCommands.php Outdated Show resolved Hide resolved
@weitzman
Copy link
Member

Could I get permission to push to your branch? Or you can accept my suggestions from the UI. This looks great to me. Lets address those Known Issues and then we can merge this.

@adamzimmermann
Copy link
Contributor Author

I won't be on my computer much today, but believe I have the permissions adjusted. I'm happy to help with all of the needed adjustments next week, but I won't be around much this weekend. Hopefully I've unblocked things though. 🤞🏼

@markdorison
Copy link

Do we think this can be the default behavior? I would love it to be, but will this change be disruptive to some?

@weitzman
Copy link
Member

I think it should only be disruptive when there is a genuine problem. Thus I think it runs always during a deploy. We can add a flag later if a need arises.

@adamzimmermann
Copy link
Contributor Author

Thank you all for chiming in and pushing this forward. 🙏🏼

I'm hoping to find some time later today to target this to the 12.x branch and improve the logging.

Anything else that I should add to this before we consider is ready for a more final review?

I might need some help with test coverage too, but I'll take a swing at it.

@apotek
Copy link
Contributor

apotek commented Jul 24, 2023

Thank you all for chiming in and pushing this forward. 🙏🏼

I'm hoping to find some time later today to target this to the 12.x branch and improve the logging.

Anything else that I should add to this before we consider is ready for a more final review?

I might need some help with test coverage too, but I'll take a swing at it.

Thank you for your great work on this @adamzimmermann . The language (output) around this will be critical to a variety of users understanding why their deployment was blocked, and it strikes me that designing the user feedback will end up being a tangle in and of itself :)

@adamzimmermann
Copy link
Contributor Author

Closing in favor of #5718.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants