Render past successful deploys on the releases overview #2897

dasch · 2018-08-24T12:49:08Z

It's valuable to be able to see at a glance whether a given release has been successfully deployed to a stage in the past, e.g. when you want to know if a build stage has completed.

The past, successful deploys are displayed with a grey background.

/cc @zendesk/samson

Tasks

👍 from team

Risks

Level: Low

ragurney · 2018-08-24T17:09:43Z

app/helpers/application_helper.rb

-      next unless deploy.references?(reference)
-      label = (deploy.active? ? "label-warning" : "label-success")
+      # The first deploy is the most recent one.
+      next unless deploy = stage.deploys.where(reference: reference).first


Instead of doing n where queries, would it be better to do something like:

deploys_hash = Deploy.joins(:stage).where(stage_id: stages.pluck(:id)).where(reference: 'master').group_by(&:stage_id) stages.each do |stage| next unless deploy = deploys_hash[stage.id]&.first ... end

?

samson(dev)> puts Benchmark.measure { deployed_or_running_list(stages, reference) } Deploy Load (0.6ms) SELECT `deploys`.* FROM `deploys` WHERE `deploys`.`deleted_at` IS NULL AND `deploys`.`stage_id` = 4 AND `deploys`.`reference` = 'master' ORDER BY `deploys`.`id` DESC LIMIT 1 Deploy Load (0.5ms) SELECT `deploys`.* FROM `deploys` WHERE `deploys`.`deleted_at` IS NULL AND `deploys`.`stage_id` = 2 AND `deploys`.`reference` = 'master' ORDER BY `deploys`.`id` DESC LIMIT 1 Deploy Load (0.4ms) SELECT `deploys`.* FROM `deploys` WHERE `deploys`.`deleted_at` IS NULL AND `deploys`.`stage_id` = 3 AND `deploys`.`reference` = 'master' ORDER BY `deploys`.`id` DESC LIMIT 1 Deploy Load (0.4ms) SELECT `deploys`.* FROM `deploys` WHERE `deploys`.`deleted_at` IS NULL AND `deploys`.`stage_id` = 1 AND `deploys`.`reference` = 'master' ORDER BY `deploys`.`id` DESC LIMIT 1 Deploy Load (0.4ms) SELECT `deploys`.* FROM `deploys` WHERE `deploys`.`deleted_at` IS NULL AND `deploys`.`stage_id` = 5 AND `deploys`.`reference` = 'master' ORDER BY `deploys`.`id` DESC LIMIT 1 Deploy Load (0.3ms) SELECT `deploys`.* FROM `deploys` WHERE `deploys`.`deleted_at` IS NULL AND `deploys`.`stage_id` = 6 AND `deploys`.`reference` = 'master' ORDER BY `deploys`.`id` DESC LIMIT 1 Deploy Load (0.4ms) SELECT `deploys`.* FROM `deploys` WHERE `deploys`.`deleted_at` IS NULL AND `deploys`.`stage_id` = 7 AND `deploys`.`reference` = 'master' ORDER BY `deploys`.`id` DESC LIMIT 1 0.005582 0.000476 0.006058 ( 0.008085) => nil samson(dev)> puts Benchmark.measure { deployed_or_running_list2(stages, reference) } Deploy Load (1.0ms) SELECT `deploys`.* FROM `deploys` INNER JOIN `stages` ON `stages`.`id` = `deploys`.`stage_id` AND `stages`.`deleted_at` IS NULL WHERE `deploys`.`deleted_at` IS NULL AND `deploys`.`stage_id` IN (4, 2, 3, 1, 5, 6, 7) AND `deploys`.`reference` = 'master' ORDER BY `deploys`.`id` DESC 0.002177 0.000184 0.002361 ( 0.003161) => nil

It depends – if we cache each row in the table based on the Release, and make sure to touch the release whenever it's deployed, then an n + 1 makes sense, as the common case will be that almost all of the releases will be cached and therefore won't trigger any queries. That's preferable to naively preloading everything.

I'd like to add that caching in a separate PR, though, unless it's a blocker for this one.

something like this ?

referenced = stage.deploys.where(ref).first deploy = stage.last_deploy if referenced == deploy if deploy.success? 'success' elsif deploy.running? 'warning' else 'default' end end

Not quite – I'll push an updated version.

ragurney · 2018-08-24T17:13:03Z

test/helpers/application_helper_test.rb

@@ -700,11 +700,23 @@ def render
      assert html.html_safe?
    end

-    it "renders succeeded deploys" do
+    it "renders current, succeeded deploys" do
      html = deployed_or_running_list(stage_list, "staging")


Might be nice to use assert_sql_queries here to lock in the number of queries in case someone comes along and changes this later.

grosser · 2018-08-27T15:32:53Z

app/helpers/application_helper.rb

+        end
+      end
+
+      if label.present?


prefer consistent if thing instead of if thing.nil? + if thing.present?

grosser · 2018-08-27T15:34:00Z

app/helpers/application_helper.rb

+      end
+
+      if label.present?
+        text = stage.name.html_safe


this smells like xss ... I can name my stage <script>foo</script>

grosser · 2018-08-27T15:38:28Z

app/helpers/application_helper.rb

-      html << " "
+    pieces = stages.map do |stage|
+      # The first deploy is the most recent one.
+      deploy = stage.deploys.where(reference: reference).first


we should fetch all the deploys in a single query with stage.project.deploys.select('max(id) as id').where(stage_id: stages.map(&:id), reference: reference).group(:stage_id) ?

Won't that just give the Deploy id, not the full deploy?

Okay, I've pushed an optimized version that only does 1 query per row.

It's valuable to be able to see at a glance whether a given release has been successfully deployed to a stage in the past, e.g. when you want to know if a build stage has completed.

Don't use a reference that's identical to the stage name, that's confusing.

dasch · 2018-08-28T12:14:49Z

@grosser one more review?

We need two queries.

grosser · 2018-08-28T15:52:42Z

app/models/deploy.rb

+      select("MAX(id) AS id, stage_id")
+
+    # Fetch the actual deploys.
+    deploys = Deploy.where(id: deploys_and_stages.map(&:id))


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

grosser · 2018-08-28T15:54:42Z

app/models/deploy.rb

+  end
+
+  # Returns a Hash of Stage => Deploy for the given reference and stages.
+  def self.of_reference_in_stages(reference, stages)


grouped hashes are usually built from methods with by not of

grosser · 2018-08-28T15:56:07Z

app/helpers/application_helper.rb

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


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)

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.

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

grosser · 2018-08-28T15:56:54Z

app/models/deploy.rb

+      [stage, deploy]
+    end.to_h
+  end
+


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

grosser · 2018-08-28T15:57:33Z

app/models/deploy.rb

@@ -171,6 +171,30 @@ def self.after(deploy)
    where("#{table_name}.id > ?", deploy.id)
  end

+  def self.in_stages(stages)
+    where(stage_id: stages.map(&:id))
+  end


kinda simple query I'd rather keep inline

grosser · 2018-08-29T16:22:02Z

app/helpers/application_helper.rb

+      elsif deploy.running?
+        "label-warning"
+      elsif deploy.succeeded?
+        if deploy == stage.last_deploy


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

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.

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

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

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 ?

dasch requested a review from ragurney August 24, 2018 12:50

ragurney reviewed Aug 24, 2018

View reviewed changes

grosser reviewed Aug 27, 2018

View reviewed changes

dasch added 10 commits August 28, 2018 13:26

Render past successful deploys on the releases overview

e14fa80

It's valuable to be able to see at a glance whether a given release has been successfully deployed to a stage in the past, e.g. when you want to know if a build stage has completed.

Fixes

dafb896

Fix the logic

0be72f7

Make the test more clear

5fa1cc0

Don't use a reference that's identical to the stage name, that's confusing.

Add test to avoid introducing new queries

05ed49c

Rubocop fix

cbf600c

Simplify HTML generation

7b74fe5

Rubocop fix

77378b1

Fix test

472863c

Feedback fixes

e52df04

dasch force-pushed the dasch/show-past-deploys branch from 443898b to e52df04 Compare August 28, 2018 11:27

Optimize the loading of deploys per release

4b13fa6

dasch added 2 commits August 28, 2018 14:47

Rubocop fix

fcd7d96

Fix the lookup method

af6e107

We need two queries.

grosser reviewed Aug 28, 2018

View reviewed changes

app/models/deploy.rb

[stage, deploy]

end.to_h

end

Copy link

Contributor

grosser Aug 28, 2018

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

grosser reviewed Aug 28, 2018

View reviewed changes

dasch added 2 commits August 29, 2018 12:36

Inline in_stages

f1f6342

Update the query counts

6ff52b3

grosser reviewed Aug 29, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Render past successful deploys on the releases overview #2897

Render past successful deploys on the releases overview #2897

dasch commented Aug 24, 2018 •

edited

Loading

ragurney Aug 24, 2018 •

edited

Loading

dasch Aug 24, 2018

grosser Aug 24, 2018 •

edited

Loading

dasch Aug 27, 2018

ragurney Aug 24, 2018

dasch Aug 27, 2018

grosser Aug 27, 2018

dasch Aug 28, 2018

grosser Aug 27, 2018

dasch Aug 28, 2018

grosser Aug 27, 2018

dasch Aug 28, 2018

dasch Aug 28, 2018

dasch commented Aug 28, 2018

grosser Aug 28, 2018

grosser Aug 28, 2018

grosser Aug 28, 2018 •

edited

Loading

dasch Aug 29, 2018

grosser Aug 29, 2018

grosser Aug 28, 2018

grosser Aug 28, 2018

dasch Aug 29, 2018

grosser Aug 29, 2018

dasch Aug 30, 2018

grosser Aug 31, 2018

dasch Aug 31, 2018

grosser Aug 31, 2018 •

edited

Loading

Render past successful deploys on the releases overview #2897

Are you sure you want to change the base?

Render past successful deploys on the releases overview #2897

Conversation

dasch commented Aug 24, 2018 • edited Loading

Tasks

Risks

ragurney Aug 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grosser Aug 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dasch commented Aug 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grosser Aug 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grosser Aug 31, 2018 • edited Loading

Choose a reason for hiding this comment

dasch commented Aug 24, 2018 •

edited

Loading

ragurney Aug 24, 2018 •

edited

Loading

grosser Aug 24, 2018 •

edited

Loading

grosser Aug 28, 2018 •

edited

Loading

grosser Aug 31, 2018 •

edited

Loading