Shopify / shipit-engine

Deployment coordination
https://shopify.engineering/introducing-shipit
MIT License
1.42k stars 144 forks source link

Change `emit_lock_hooks` to not check for `previous_changes` in order to enable it running after "Abort & Rollback" is clicked #1270

Closed kwboyd-shopify closed 2 years ago

kwboyd-shopify commented 2 years ago

What: This gets rid of the automatic return in emit_lock_hooks if previous_changes doesn't contain a change to lock_reason.

Why: Right now, hitting "Abort & Rollback" on a deploy does not actually end up emitting a lock hook. This is because of this behavior in ActiveRecord. previous_changes was ending up empty when emit_lock_hooks was actually called in the callbacks.

Risks: This does mean we'll be emitting a lot more "lock" webhooks, even when the lock has not changed. Are we okay with this, or would we prefer one of the alternatives below?

Alternatives:

casperisfine commented 2 years ago

This does mean we'll be emitting a lot more "lock" webhooks, even when the lock has not changed. Are we okay with this

I don't think we're OK with this. It will mess with chat notifications.

Can we just do an ad hoc fix (e.g. emit a hook after a rollback is created?)