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
Open
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.
+83 −13
Diff settings

Always

Just for now

Next

Pass rollbackability from Add to Merge Queue step

  • Loading branch information...
JackTLi committed Apr 4, 2019
commit 7bb038ee2ac727a64daeb553df895c501b2b2f83
@@ -30,7 +30,7 @@ class MergeStatusPoller

previousLastModified = null
refreshPage: =>
@fetchPage window.location.toString(), (html, response) =>
@fetchPage @reloadUrl(), (html, response) =>
@updateDocument(html, response)
setTimeout(@refreshPage, POLL_INTERVAL)

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

reloadUrl: =>
rollback_checkbox = document.querySelector('input[name="rollbackable"]:checked')
This conversation was marked as resolved by JackTLi

This comment has been minimized.

Copy link
@casperisfine

casperisfine Apr 9, 2019

Contributor

So as you figured, this part of the UI really wasn't meant to hold state.

That being said, if you want to do this, a cleaner way could be to serialize the form in a more generic way, e.g. get the form element and iterate over the inputs and build the query string.

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.


isMergeQueueEnabled: =>
document.querySelector('.merge-status-container .js-details-container')?.hasAttribute('data-queue-enabled')

@@ -153,6 +153,14 @@
color: $red;
}

.rollbackable-yes {
color: $green;
}

.rollbackable-no {
color: $red;
}


// COMMIT ACTIONS
// -----------------------------------------------------------------------------
@@ -1,3 +1,10 @@
.merge-status-container {
max-width: 700px;
}

.rollbackability {
margin-bottom: 15px;
input {
margin-left: 10px;
}
}
@@ -22,7 +22,7 @@ def show
end

def enqueue
PullRequest.request_merge!(stack, params[:number], current_user)
PullRequest.request_merge!(stack, params[:number], current_user, params[:rollbackable])
render stack_status, layout: !request.xhr?
end

@@ -257,6 +257,7 @@ def identify_pull_request
self.pull_request_number = pull_request.number
self.pull_request_title = pull_request.title
self.author = pull_request.merge_requested_by if pull_request.merge_requested_by
self.rollbackable = pull_request.rollbackable
end

self.pull_request_number = message_parser.pull_request_number unless self[:pull_request_number]
@@ -119,20 +119,21 @@ def self.extract_number(stack, number_or_url)
end
end

def self.request_merge!(stack, number, user)
def self.request_merge!(stack, number, user, rollbackable)
now = Time.now.utc
pull_request = begin
create_with(
merge_requested_at: now,
merge_requested_by: user.presence,
rollbackable: rollbackable
).find_or_create_by!(
stack: stack,
number: number,
)
rescue ActiveRecord::RecordNotUnique
retry
end
pull_request.update!(merge_requested_by: user.presence)
pull_request.update!(merge_requested_by: user.presence, rollbackable: rollbackable)
pull_request.retry! if pull_request.rejected? || pull_request.canceled? || pull_request.revalidating?
pull_request.schedule_refresh!
pull_request
@@ -17,6 +17,15 @@
<p class="commit-meta">
<%= timeago_tag(commit.committed_at, force: true) %>
</p>
<p class="commit-meta">
<% if commit.rollbackable%>
<span class='rollbackable-yes'> Safe to rollback </span>
This conversation was marked as resolved by JackTLi

This comment has been minimized.

Copy link
@casperisfine

casperisfine Apr 9, 2019

Contributor

For a boolean input, a checkbox would make much more sense IMO.

This comment has been minimized.

Copy link
@JackTLi

JackTLi Apr 9, 2019

Author Member

hmm could you expand on this? I can't quite visualize what you're describing 😛

This comment has been minimized.

Copy link
@casperisfine

casperisfine Apr 9, 2019

Contributor

It's because I commented on the wrong diff, I meant to comment on the part where the HCTW extension use radio buttons to ask for yes/no.

This comment has been minimized.

Copy link
@JackTLi

JackTLi Apr 9, 2019

Author Member

Ah gotcha. I don't mind that actually - just moving the PR checkbox down. Would make it easier too if we do the unsafe_to_rollback rename

<% 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?

<% end %>
</p>
</div>
<div class="commit-lock" >
<%= link_to stack_commit_path(commit.stack, commit), class: 'action-lock-commit', data: {tooltip: t('commit.lock')} do %>
@@ -1,4 +1,4 @@
<% classes = 'branch-action-btn float-right js-immediate-updates js-handle-pull-merging-errors' %>
<% classes = 'js-immediate-updates js-handle-pull-merging-errors' %>
<% if pull_request.try!(&:waiting?) %>
<%= form_tag dequeue_pull_request_path(stack, pull_request_number), method: 'delete', class: classes, data: {remote: true} do %>
<%= hidden_field_tag 'referrer', params[:referrer] %>
@@ -9,8 +9,24 @@
<% else %>
<%= form_tag enqueue_pull_request_path(stack, pull_request_number), method: 'put', class: classes, data: {remote: true} do %>
<%= hidden_field_tag 'referrer', params[:referrer] %>
<button type="submit" data-disable-with="Adding to merge queue…" class="btn btn-primary">
Add to merge queue
</button>

<div class='rollbackability'>
<%= label_tag :rollbackable do %>
Is this change <%= link_to('safe to rollback?', 'https://development.shopify.io/guides/shipping/safe_to_merge#Signs_your_PR_is_safe_to_rollback', target: :_blank) %>
This conversation was marked as resolved by JackTLi

This comment has been minimized.

Copy link
@JackTLi

JackTLi Apr 4, 2019

Author Member

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

<% end %>
<% if params[:rollbackable] == 'false' %>
<%= radio_button_tag :rollbackable, true %> Yes
<%= radio_button_tag :rollbackable, false, checked: true %> No
<% else %>
<%= radio_button_tag :rollbackable, true, checked: true %> Yes
<%= radio_button_tag :rollbackable, false %> No
<% end %>
</div>

<span class='branch-action-btn float-right'>
<button type="submit" data-disable-with="Adding to merge queue…" class="btn btn-primary">
Add to merge queue
</button>
</span>
<% end %>
<% end %>
@@ -17,6 +17,15 @@
<em class="warning">Need revalidation.</em>
<% end %>
</p>
<p class="pr-meta">
<% if pull_request.rollbackable %>
<span class='rollbackable-yes'> Safe to rollback </span>
<% elsif pull_request.rollbackble == false %>
<span class='rollbackable-no'> Unsafe to rollback </span>
<% else %>
<!-- Do not display anything if rollbackable is nil -->
<% end %>
</p>
</div>
<% if pull_request.revalidating? %>
<div class="commit-actions">
@@ -0,0 +1,5 @@
class AddRollbackableToCommits < ActiveRecord::Migration[5.2]
def change
add_column :commits, :rollbackable, :boolean
end
end
@@ -0,0 +1,5 @@
class AddRollbackableToPullRequests < ActiveRecord::Migration[5.2]
def change
add_column :pull_requests, :rollbackable, :boolean
end
end
@@ -10,7 +10,7 @@
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema.define(version: 2018_10_10_150947) do
ActiveRecord::Schema.define(version: 2019_04_04_184348) do

create_table "api_clients", force: :cascade do |t|
t.text "permissions", limit: 65535
@@ -76,6 +76,7 @@
t.string "pull_request_title", limit: 1024
t.integer "pull_request_id"
t.boolean "locked", default: false, null: false
t.boolean "rollbackable"
t.index ["author_id"], name: "index_commits_on_author_id"
t.index ["committer_id"], name: "index_commits_on_committer_id"
t.index ["created_at"], name: "index_commits_on_created_at"
@@ -165,6 +166,7 @@
t.datetime "merged_at"
t.string "base_ref", limit: 1024
t.integer "base_commit_id"
t.boolean "rollbackable"
t.index ["head_id"], name: "index_pull_requests_on_head_id"
t.index ["merge_requested_by_id"], name: "index_pull_requests_on_merge_requested_by_id"
t.index ["merge_status"], name: "index_pull_requests_on_merge_status"
@@ -184,9 +186,9 @@
t.bigint "github_id"
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
t.index ["commit_id", "github_id"], name: "index_deploy_statuses_on_commit_id_and_github_id"
t.index ["stack_id", "commit_id"], name: "index_deploy_statuses_on_stack_id_and_commit_id"
t.index ["user_id"], name: "index_deploy_statuses_on_user_id"
t.index ["commit_id", "github_id"], name: "index_release_statuses_on_commit_id_and_github_id"
t.index ["stack_id", "commit_id"], name: "index_release_statuses_on_stack_id_and_commit_id"
t.index ["user_id"], name: "index_release_statuses_on_user_id"
end

create_table "stacks", force: :cascade do |t|
@@ -239,7 +241,7 @@
t.integer "additions", limit: 4, default: 0
t.integer "deletions", limit: 4, default: 0
t.text "definition", limit: 65535
t.binary "gzip_output"
t.binary "gzip_output", limit: 16777215
t.boolean "rollback_once_aborted", default: false, null: false
t.text "env"
t.integer "confirmations", default: 0, null: false
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.