Commit locking system is confusing

Shopify / shipit-engine

Deployment coordination

https://shopify.engineering/introducing-shipit

MIT License

1.41k stars 145 forks source link

Commit locking system is confusing #850

Open eapache opened 5 years ago

eapache commented 5 years ago

@byroot @qq99 @NatMorcos we discovered today that the commit locking system is rather confusing, because locking a single commit as bad doesn't prevent a deploy going out with that commit included. The simplest fix is likely:

when manually marking a commit as bad, all commits after it should be marked as bad until a second commit is manually marked as good

qq99 commented 5 years ago

I don't understand the UX of it, I couldn't really dig in at the time due to ongoing incident but, given the example stack of: X, Y, Z (where X is oldest merged and Z is recently merged)

If I mark X as unsafe, then I would expect nothing else to need to be marked

I would expect that Y and Z will not deploy since:

to deploy Y, you must deploy X (because it's a stack)
to deploy Z, you must deploy Y, you must deploy X (because it's a stack)
you cannot deploy X because it was unsafe
therefore marking X as unsafe is all that should be needed to be done; X (explicitly locked), and anything after X is implicitly locked
shipping commits A..W earlier in the stack is still fine

Why should Y and Z also need to be marked unsafe (UX-wise?)

casperisfine commented 5 years ago

Why should Y and Z also need to be marked unsafe (UX-wise?)

Because as you said a commit log is an append only stack/log. So there is absolutely no way to know what commit will make the deploy safe again, you have to explicitly tell Shipit which range is broken.

There is an heuristic in place to detect straight reverts, and automatically lock the range between the original commit and it's revert, but we can't do more than that.

qq99 commented 5 years ago

I think I see what you mean now. I still believe that marking X as unsafe should prevent X or its descendents from deploying, is this not what happens today? If not, it's not really intuitive.

UX-wise, could it be framed as marking Z as explicitly safe (s.t. it knows X or XY is not OK, but XYZ is OK?)

I guess you really need a pointer from the unsafe commit to the commit that makes it safe again (in cases where you have two unsafe commits on the stack at a time with 2 fixes)

What about an interim tweak: If I mark X as unsafe in the UI, it automatically marks X,Y,Z,Z',Z'',etc as unsafe for me too? Assuming this still doesn't help you if someone merges and you're not locked

eapache commented 5 years ago

Ya, that's what I was proposing with

when manually marking a commit as bad, all commits after it should be marked as bad until a second commit is manually marked as good