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

Track rollback safeness in shipit #889

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@JackTLi
Copy link
Member

JackTLi commented Apr 4, 2019

Button:

image

On undeployed commits list:

image

Shipit stack:

image

In this case, add ergodox config is the deploy which contains a unrollbackable commit. This means that we can rollback to this revision, but any rollback attempt on an older revision will result in the following screen:

image

Imo we should always give developers the option to rollback a commit even if it's marked as unsafe, because it's possible that it was marked as unsafe wrongly. At the same time, we should raise sufficient alarms so that these actions are performed with the right information. Any thoughts on the above ui/experience? Should we make it louder?

cc @Shopify/pipeline

@JackTLi JackTLi force-pushed the shipit-rollbackability branch from f5837b0 to a753a69 Apr 4, 2019

@JackTLi JackTLi force-pushed the shipit-rollbackability branch 3 times, most recently from f95ba5e to 0dd8fd5 Apr 4, 2019

@JackTLi JackTLi force-pushed the shipit-rollbackability branch from 0dd8fd5 to 7bb038e Apr 4, 2019

@JackTLi JackTLi requested review from DazWorrall and esnunes Apr 4, 2019

if rollback_checkbox == null
return window.location.toString()
else
return window.location.toString() + "&rollbackable=" + rollback_checkbox.value

This comment has been minimized.

Copy link
@JackTLi

JackTLi Apr 4, 2019

Author Member

The page refresh/poll here gets in the way a little bit, where after "no" is selected, the autoupdate can kick in which resets the radio buttons. '

This is the workaround, which will stick the current radio button value to the end of the refresh link as a query parameter, and then when we render the html.erb for the page, we will render the html with a default checked radio depending on this param. Since the logic here does a strict html replace, it'll ensure the "current" radio button selection carries through without having to do any retrospective dom manipulation on each refresh.

I spent way too long banging my head for a good solution to this, and this seems to be the least intrusive. lmk if anyone has any suggestions

This comment has been minimized.

Copy link
@esnunes

esnunes Apr 5, 2019

Contributor

one potential solution is: store the state of the rollbackable flag in the hash part of the url

This comment has been minimized.

Copy link
@JackTLi

JackTLi Apr 5, 2019

Author Member

I think you're describing the behaviour here - I'm storing the state in the url as something like:
shipit.shopify.io/merge_status?commits=1&rollbackable=true

This comment has been minimized.

Copy link
@mcgain

mcgain Apr 11, 2019

Member

you could also use window.localStorage if you don't want the info in the url. More stuff like expiration to deal with then though.

@JackTLi JackTLi changed the title [WIP] Pass rollbackability from Add to Merge Queue step [WIP] Track rollbackability in shipit Apr 5, 2019

@DazWorrall
Copy link
Member

DazWorrall left a comment

Promising start! Will the render in the Github UI ok? Not sure about 'rollbackable' as a term but let's focus on function first :p

@DazWorrall DazWorrall requested a review from casperisfine Apr 8, 2019

@JackTLi

This comment has been minimized.

Copy link
Member Author

JackTLi commented Apr 8, 2019

It'll render in the github UI exactly as the screenshot above, the screenshotted page was rendered with github's stylesheet that we inject :)

also yeah, I ran into a snag with rollbackability since it's already something that's used on deploy. I'm probably gonna rename the flag to safe_to_deploy instead. Thoughts?

Also added the experience on rollbacks - Logic and the associated screenshots are documented in the pr summary.

@casperisfine
Copy link
Contributor

casperisfine left a comment

I think this PR is confronted to the most difficult Shipit issue, user input in this flow is very hard.

Show resolved Hide resolved app/assets/javascripts/merge_status.coffee Outdated
Show resolved Hide resolved app/models/shipit/deploy.rb Outdated
Show resolved Hide resolved app/models/shipit/deploy.rb Outdated
Show resolved Hide resolved app/views/shipit/commits/_commit.html.erb Outdated
<% elsif commit.rollbackable == false %>
<span class='rollbackable-no'> Unsafe to rollback </span>
<% else %>
<!-- Do not display anything if rollbackable is nil -->

This comment has been minimized.

Copy link
@casperisfine

casperisfine Apr 9, 2019

Contributor

Tri-state boolean tend to be a smell. Why would a commit be rollbackable=nil ? Can't we default to either true or false ?

This comment has been minimized.

Copy link
@JackTLi

JackTLi Apr 9, 2019

Author Member

Although we can treat nil functionally the same as the true case, I don't want to surface it to the ui if it's not explicitly set. i.e. if a merge is done without going through the merge queue flow, we won't have this value, or for repos that don't have the merge queue enabled. I could maybe convert this to column to unsafe_to_rollback so that at least the false and nil cases can be used interchangeably for most of the logic?


<div class='rollbackability'>
<%= label_tag :unsafe_to_rollback do %>
This change is <%= link_to('unsafe to rollback', 'https://development.shopify.io/guides/shipping/safe_to_merge#Signs_your_PR_is_safe_to_rollback', target: :_blank) %>

This comment has been minimized.

Copy link
@JackTLi

JackTLi Apr 9, 2019

Author Member

Todo: Find a public source or make this link configurable, as the current link is only available to shopify employees

@JackTLi JackTLi force-pushed the shipit-rollbackability branch from 91fafbd to d5c562f Apr 9, 2019

@JackTLi JackTLi force-pushed the shipit-rollbackability branch from d5c562f to c29f94e Apr 9, 2019

@JackTLi JackTLi changed the title [WIP] Track rollbackability in shipit Track rollbackability in shipit Apr 9, 2019

@JackTLi JackTLi changed the title Track rollbackability in shipit Track rollback safeness in shipit Apr 9, 2019

@JackTLi

This comment has been minimized.

Copy link
Member Author

JackTLi commented Apr 10, 2019

@DazWorrall @casperisfine @esnunes would appreciate another round of 👀

@casperisfine
Copy link
Contributor

casperisfine left a comment

I'm still uneasy with the nullable boolean, but I don't have all the context so if you feel it's the way 👍

Apart from that:

  • A small naming issuee
  • The checkbox persistence needs some thoughts. I brain dumped a bit about it, but not sure I see good solutions just now.
@@ -39,6 +39,16 @@ def append_status(task_status)

delegate :broadcast_update, :filter_deploy_envs, to: :stack

def self.newer_than(commit)
return all unless commit
where('id > ?', commit.try(:id) || commit)

This comment has been minimized.

Copy link
@casperisfine

casperisfine Apr 10, 2019

Contributor

I suppose you copy pasted this from Commit, because id in this context is a deploy.id, so that argument should probably be deploy

@@ -42,6 +42,10 @@ class MergeStatusPoller
container.innerHTML = html
@onPageChange()

reloadUrl: =>
mergeForm = document.querySelector('form#merge-form')

This comment has been minimized.

Copy link
@casperisfine

casperisfine Apr 10, 2019

Contributor

Ok, so you are going to hate me because I told you to do this.

Turns out the form contains these 3 hidden fields:

 <input name="utf8" type="hidden" value="✓">
 <input type="hidden" name="_method" value="put">
 <input type="hidden" name="authenticity_token" value="[REDACTED]">

The first two we could almost ignore, it's just ugly but fine.

But the 3rd one is different on every request, which mean we'd totally break the HTTP caching.

Also that same caching might cause you troubles, you'll need to include any form attribute you want to persist in there.

Really the HCTW iframe is not well suited for stateful inputs :/

I honestly have no idea how to implement that without lots of warts.

Maybe that data shouldn't be included in the URL, but reset by the javascript in the fetchPage callback?

This comment has been minimized.

Copy link
@JackTLi

JackTLi Apr 10, 2019

Author Member

So I didn't go through your suggestion fully, I'm using this to serialize the form state into the query string but I'm only using the single unsafe_to_rollback value to seed the initial checkbox state. This is the page that the refresh mechanism polls, I'm not manipulating the url at all for the original page.

The rest of the query params are actually just ignored, including the authenticity token.

&yeah I totally found that this wasn't meant for stateful inputs. Hopefully this isn't too nasty of a workaround though.

@DazWorrall

This comment has been minimized.

Copy link
Member

DazWorrall commented Apr 11, 2019

I'm really tired and my head is full of conference things so I apologise in advance for the half baked thoughts.

I'm worried about unintended consequences / perverse incentives with this change, and want to make sure we're aligned on what we want to achieve. The way I see it there are 2 big areas we want a better story on:

  • When things are on fire, people dealing with the issue may not have all the context on all the changes. A quick signal on rollback safety allows them to move more quickly and more confidently at at time when they may really need to
  • Have developers think about rollback safety, make a conscious decision either way, and then optionally / later have Shipit help them rollout unrollbackable changes in isolation (so that they're not bundled with other changes, which might break, and need rolling back)

If we're agreed there / I haven't missed anything, I wonder if we shouldn't actually have a default set either way, and have people deliberately pick - and refuse to merge without a choice. On the plus side it cant be ignored, and we sidestep the risk that we don't trust the boolean because people never set it. On the downside, shipping an unrollbackable change should be extremely exceptional, and so for the vast majority of PR's we're introducing unnecessary friction. I'm keen to get everyone's thoughts on this.

@JackTLi

This comment has been minimized.

Copy link
Member Author

JackTLi commented Apr 11, 2019

Thanks for the comments everybody, I think we can make the "Add to Merge Queue" a two step process like this:

[Add to merge queue]
⬇️
click
⬇️
Is this change safe to rollback? [Yes] [No]
⬇️
click yes or no
⬇️
change added to merge queue with flag set

This change will also fix the polling/state problem.

We can make it so that clicking the initial "Add to merge queue" button pauses the polling. Once Yes/No are selected, we can resume the polling. We could also put the second step here on a timer, so that if Yes/No is not selected within a certain time, the polling kicks in again (which will reset the ui back to "add to merge queue). Maybe a timer bar at the bottom can communicate this. This will make it so that there's a maximum time drift between Shipit's state and the user's UI

@Shopify/pipeline thoughts?

@doodzik

This comment has been minimized.

Copy link
Member

doodzik commented Apr 12, 2019

I like that idea and that it is process driven. Maybe add this to the shipitnext document for experimentation.

@JackTLi JackTLi self-assigned this Apr 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.