-
Notifications
You must be signed in to change notification settings - Fork 235
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
base: master
Are you sure you want to change the base?
Changes from all commits
e14fa80
dafb896
0be72f7
5fa1cc0
05ed49c
cbf600c
7b74fe5
77378b1
472863c
e52df04
4b13fa6
fcd7d96
af6e107
f1f6342
6ff52b3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
|
||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can't we build the same map for ... for another PR: we could also prefetch the jobs for all deploys to save the job lookups that are caused by the status calls There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean – once you've called There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, but it's being done once per stage -> n+1 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) ?