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

Render past successful deploys on the releases overview #2897

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
39 changes: 27 additions & 12 deletions app/helpers/application_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -322,20 +322,35 @@ def link_to_chart(name, values)
link_to icon_tag('signal'), url, target: :blank
end

# show which stages this reference is deploy(ed+ing) to
# Show which stages this reference has been or is currently being deployed to.
def deployed_or_running_list(stages, reference)
html = "".html_safe
stages.each do |stage|
next unless deploy = stage.deployed_or_running_deploy
next unless deploy.references?(reference)
label = (deploy.active? ? "label-warning" : "label-success")

text = "".html_safe
text << stage.name
html << content_tag(:span, text, class: "label #{label} release-stage")
html << " "
deploys = Deploy.of_reference_in_stages(reference, stages)
Copy link
Contributor

@grosser grosser Aug 28, 2018

Choose a reason for hiding this comment

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

deploy_lookup = Deploy.where(reference: reference).where(stage_id: stages.map(&:id)).group(:stage_id).select("MAX(id) AS id, stage_id").group_by(&:stage_id)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I really dislike putting SQL in the view layer – it makes testing so much harder. I'd very much prefer to keep this in the model.

Copy link
Contributor

Choose a reason for hiding this comment

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

the query logic is closely tied to the compare logic, so could lead to bugs/get out of sync when it's extracted to the model ... but I don't care that much
more important: the group_by line seems much easier to read (assuming it works as before) ?


pieces = stages.map do |stage|
deploy = deploys[stage]

label = if deploy.nil?
nil
elsif deploy.running?
"label-warning"
elsif deploy.succeeded?
if deploy == stage.last_deploy
Copy link
Contributor

Choose a reason for hiding this comment

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

can't we build the same map for all last deploys from these stages ... so we end up with 2 queries ? (+ job fetching queries)

... for another PR: we could also prefetch the jobs for all deploys to save the job lookups that are caused by the status calls

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean – once you've called Stage#last_deploy, that should be memoized, right? If it's not, I can easily add that. The stages don't change between the release rows, so we shouldn't need to do any fancy work there.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, but it's being done once per stage -> n+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How many stages do we typically have? Is it worth the added complexity?

Copy link
Contributor

@grosser grosser Aug 31, 2018

Choose a reason for hiding this comment

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

projects can have tons of stages ... this PR is not making it worse, so keep it like it is and open a BRE issue to make this more performant ?

"label-success"
else
# Deploy is neither active nor is it the last successful one, but it
# succeeded in the past.
"label-default"
end
end

if label
text = "".html_safe
text << stage.name
content_tag(:span, text, class: "label #{label} release-stage")
end
end
html

safe_join(pieces, " ")
end

def check_box_section(section_title, help_text, object, method, collection)
Expand Down
20 changes: 20 additions & 0 deletions app/models/deploy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,26 @@ def self.after(deploy)
where("#{table_name}.id > ?", deploy.id)
end

# Returns a Hash of Stage => Deploy for the given reference and stages.
def self.of_reference_in_stages(reference, stages)
# Group by stage, then select the latest deploy id.
deploys_and_stages = where(reference: reference).
where(stage_id: stages.map(&:id)).
group(:stage_id).
select("MAX(id) AS id, stage_id")

# Fetch the actual deploys.
deploys = Deploy.where(id: deploys_and_stages.map(&:id))
Copy link
Contributor

Choose a reason for hiding this comment

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

we only use the id and stage_id ... so we could avoid an extra query ?


# Map to a hash of stage => deploy entries.
deploys.map do |deploy|
# Don't trigger another query in order to fetch the stage.
stage = stages.find { |stage| stage.id == deploy.stage_id }

[stage, deploy]
end.to_h
end

Copy link
Contributor

Choose a reason for hiding this comment

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

all pretty funky and not reused, so I'd keep it inline

def self.expired
threshold = BuddyCheck.time_limit.ago
stale = where(buddy_id: nil).joins(:job).where(jobs: {status: 'pending'}).where("jobs.created_at < ?", threshold)
Expand Down
56 changes: 46 additions & 10 deletions test/helpers/application_helper_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -698,34 +698,70 @@ def render

describe "#deployed_or_running_list" do
let(:stage_list) { [stages(:test_staging)] }
let!(:deploy) { create_deploy "v1" }

def create_deploy(reference)
stages(:test_staging).deploys.create!(
project: projects(:test),
reference: reference,
job: jobs(:succeeded_test)
)
end

it "produces safe output" do
html = deployed_or_running_list([], "foo")
html.must_equal ""
assert html.html_safe?
end

it "renders succeeded deploys" do
html = deployed_or_running_list(stage_list, "staging")
html.must_equal "<span class=\"label label-success release-stage\">Staging</span> "
it "renders current, succeeded deploys" do
html = deployed_or_running_list(stage_list, "v1")
html.must_equal "<span class=\"label label-success release-stage\">Staging</span>"
end

it "renders past, succeeded deploys" do
# This deploy will be newer.
create_deploy "v2"

html = deployed_or_running_list(stage_list, "v1")
html.must_equal "<span class=\"label label-default release-stage\">Staging</span>"
end

it "ignores failed deploys" do
deploys(:succeeded_test).job.update_column(:status, 'failed')
html = deployed_or_running_list(stage_list, "staging")
deploy.job.update_column(:status, "failed")
html = deployed_or_running_list(stage_list, "v1")
html.must_equal ""
end

it "ignores non-matching deploys" do
deploys(:succeeded_test).update_column(:reference, 'nope')
html = deployed_or_running_list(stage_list, "staging")
html = deployed_or_running_list(stage_list, "yolo")
html.must_equal ""
end

it "shows active deploys" do
deploys(:succeeded_test).job.update_column(:status, 'running')
html = deployed_or_running_list(stage_list, "staging")
html.must_equal "<span class=\"label label-warning release-stage\">Staging</span> "
deploy.job.update_column(:status, 'running')
html = deployed_or_running_list(stage_list, "v1")
html.must_equal "<span class=\"label label-warning release-stage\">Staging</span>"
end

it "uses 4 queries when the deploy is the most recent one on the stage" do
assert_sql_queries 4 do
deployed_or_running_list(stage_list, "v1")
end
end

it "uses 3 queries if the deploy is currently running" do
deploy.job.update_column(:status, "running")

assert_sql_queries 3 do
deployed_or_running_list(stage_list, "v1")
end
end

it "uses 2 queries if there have been no deploys of that reference" do
assert_sql_queries 2 do
deployed_or_running_list(stage_list, "yolo")
end
end
end

Expand Down
42 changes: 42 additions & 0 deletions test/models/deploy_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -656,6 +656,39 @@ def deploy
end
end

describe "#of_reference_in_stages" do
it "returns the latest deploy with the given reference in the specified stages" do
stage1 = create_stage!(name: "stage1")
stage2 = create_stage!(name: "stage2")
stage3 = create_stage!(name: "stage3")

create_deploy!(stage: stage1, reference: "v42")
deploy1 = create_deploy!(stage: stage1, reference: "v42")
deploy2 = create_deploy!(stage: stage2, reference: "v42")
create_deploy!(stage: stage3, reference: "v42")

expected = {
stage1 => deploy1,
stage2 => deploy2
}

Deploy.of_reference_in_stages("v42", [stage1, stage2]).must_equal expected
end

it "uses only two queries" do
stage1 = create_stage!(name: "stage1")
stage2 = create_stage!(name: "stage2")

create_deploy!(stage: stage1, reference: "v42")
create_deploy!(stage: stage1, reference: "v42")
create_deploy!(stage: stage2, reference: "v42")

assert_sql_queries 2 do
Deploy.of_reference_in_stages("v42", [stage1, stage2])
end
end
end

describe "#as_json" do
it "includes simple methods status" do
deploy.as_json.fetch("status").must_equal "succeeded"
Expand Down Expand Up @@ -690,4 +723,13 @@ def create_job!(attrs = {})

Job.create!(default_attrs.merge(attrs))
end

def create_stage!(attrs = {})
default_attrs = {
project: project,
name: "some-stage"
}

Stage.create!(default_attrs.merge(attrs))
end
end