Commit b777bd73 authored by Rémy Coutable's avatar Rémy Coutable

Merge branch '22655-deployments-don-t-always-have-keep-around-refs' into 'master'

Handle case where deployment ref no longer exists

## What does this MR do?

In 8.9, we didn't create keep-around refs for deployments. So it's possible that someone created a deployment (say, for testing), and then deleted the branch and all other references to that commit. That commit could then get GCed, and trying to view MRs on 8.11+ will show a 500. See https://gitlab.com/gitlab-org/gitlab-ce/issues/22655#note_16575020 for more details.

## Why was this MR needed?

If someone created a deployment on 8.9, then deleted all references to the commit for that deployment, we will throw an exception when checking if the deployment includes a commit.

Closes #22655.

See merge request !6855
Signed-off-by: default avatarRémy Coutable <remy@rymai.me>
parent 42195ff6
...@@ -8,6 +8,7 @@ v 8.12.7 ...@@ -8,6 +8,7 @@ v 8.12.7
- Fix JS bug with select2 because of missing `data-field` attribute in select box. !6812 - Fix JS bug with select2 because of missing `data-field` attribute in select box. !6812
- Do not alter 'force_remove_source_branch' options on MergeRequest unless specified. !6817 - Do not alter 'force_remove_source_branch' options on MergeRequest unless specified. !6817
- Fix GFM autocomplete setup being called several times. !6840 - Fix GFM autocomplete setup being called several times. !6840
- Handle case where deployment ref no longer exists. !6855
v 8.12.6 v 8.12.6
- Update mailroom to 0.8.1 in Gemfile.lock !6814 - Update mailroom to 0.8.1 in Gemfile.lock !6814
......
...@@ -40,7 +40,14 @@ class Deployment < ActiveRecord::Base ...@@ -40,7 +40,14 @@ class Deployment < ActiveRecord::Base
def includes_commit?(commit) def includes_commit?(commit)
return false unless commit return false unless commit
project.repository.is_ancestor?(commit.id, sha) # Before 8.10, deployments didn't have keep-around refs. Any deployment
# created before then could have a `sha` referring to a commit that no
# longer exists in the repository, so just ignore those.
begin
project.repository.is_ancestor?(commit.id, sha)
rescue Rugged::OdbError
false
end
end end
def update_merge_request_metrics! def update_merge_request_metrics!
......
...@@ -38,5 +38,14 @@ describe Deployment, models: true do ...@@ -38,5 +38,14 @@ describe Deployment, models: true do
expect(deployment.includes_commit?(commit)).to be true expect(deployment.includes_commit?(commit)).to be true
end end
end end
context 'when the SHA for the deployment does not exist in the repo' do
it 'returns false' do
deployment.update(sha: Gitlab::Git::BLANK_SHA)
commit = project.commit
expect(deployment.includes_commit?(commit)).to be false
end
end
end end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment