Add API for manually creating deployments

changed milestone to %12.4

added api deploy devopsrelease [DEPRECATED] typefeature + 1 deleted label

	1 Warning
	This merge request is quite big (more than 1090 lines changed), please consider splitting it into multiple merge requests.

Reviewer roulette

Changes that require review have been detected! A merge request is normally reviewed by both a reviewer and a maintainer in its primary category (e.g. frontend or backend), and by a maintainer in all other categories.

To spread load more evenly across eligible reviewers, Danger has randomly picked a candidate for each review slot. Feel free to override this selection if you think someone else would be better-suited, or the chosen person is unavailable.

Once you've decided who will review this merge request, mention them as you normally would! Danger does not (yet?) automatically notify them for you.

Category	Reviewer	Maintainer
backend	Albert Salim (`@caalberts`)	Nick Thomas (`@nick.thomas`)
frontend	Vitaly Slobodin (`@vitallium`)	Kushal Pandya (`@kushalpandya`)
test for `spec/features/*`	No reviewer available	No maintainer available

Generated by Danger

resolved all threads

added 1 commit

2f94bbda - Add API for manually creating deployments

Compare with previous version

added 1 commit

e2a3f552 - Add API for manually creating deployments

Compare with previous version

added 1 commit

97abda35 - Add API for manually creating deployments

Compare with previous version

marked the checklist item Add tests for the API code as completed

added 1 commit

ef2f445c - Add API for manually creating deployments

Compare with previous version

added 378 commits

ef2f445c...c754f4ad - 377 commits from branch master
8c49ffbb - Add API for manually creating deployments

Compare with previous version

unmarked as a Work In Progress

added 1 commit

a6e9a73f - Add API for manually creating deployments

Compare with previous version

marked the checklist item Update API documentation as completed

marked the checklist item Changelog entry as completed

marked the checklist item Documentation created/updated or follow-up review issue created as completed

marked the checklist item Separation of EE specific content as completed

marked the checklist item Database guides as completed

marked the checklist item Style guides as completed

marked the checklist item Merge request performance guidelines as completed

marked the checklist item Code review guidelines as completed

added 1 commit

7ee85715 - Add API for manually creating deployments

Compare with previous version

added 1 commit

b1d3f5e8 - Add API for manually creating deployments

Compare with previous version

added 1 commit

5bbc1cd3 - Show deployment status for deployments

Compare with previous version

@nolith @rspeicher While I look into cleaning up the rendering of the deployment status, could you start taking a look at these changes and some of the questions I submitted earlier? Thanks!

added 282 commits

5bbc1cd3...67353ae2 - 281 commits from branch master
d803e6ef - Add API for manually creating deployments

Compare with previous version

changed the description

added 1 commit

01ac0ce0 - Add API for manually creating deployments

Compare with previous version

@yorickpeterse I'd still like to review this further but added a few comments for now so I don't block you, as I didn't have a chance to get to this in depth today. Looks like a good direction though.

added 176 commits

01ac0ce0...62235af2 - 175 commits from branch master
cd237e0c - Add API for manually creating deployments

Compare with previous version

added 1 commit

1c60ddfd - Add API for manually creating deployments

Compare with previous version

changed the description

added 1 commit

f99a6447 - Add API for manually creating deployments

Compare with previous version

@dosuken123 Since you have worked with the deployments code in the past, could you take a look at these changes and give any feedback? Thanks!

added backend frontend labels

@farias-gl could you take a look at the frontend/view changes? Thanks!

@tauriedavis (since you are covering for @rverissimo) Could you take a look at the UI side of things? There is a screenshot in the description that shows the UI changes. There are no other UI changes outside of the ones shown in the screenshot. Thanks!

added 1 commit

4c91ae0d - Add API for manually creating deployments

Compare with previous version

changed the description

Apparently using a feature flag inside associations causes test failures such as the following:

I am not sure how we could work around this

@rspeicher Do you perhaps have an idea on how to solve this?

@yorickpeterse I tried my best to understand the changes. This area of the codebase I have not worked with before. Either way, left some suggestions.

approved this merge request

@yorickpeterse Thanks. This is awesome feature. I couldn't have visited all code as it's already quite big MR, but will try to allocate more time to take a look more closely. For now I've left a couple of suggestions and security concerns. Let me know what you think.

changed the description

added 134 commits

4c91ae0d...2f9fef78 - 133 commits from branch master
3493a0b2 - Add API for manually creating deployments

Compare with previous version

mentioned in merge request !17645 (merged)

added direction label

Nice improvement!

added 127 commits

3493a0b2...f74cc6c5 - 126 commits from branch master
cbebd2d4 - Add API for manually creating deployments

Compare with previous version

@yorickpeterse Probably, we should still use success scope in the following code. It could affect monitoring metrics.

diff --git a/app/controllers/projects/deployments_controller.rb b/app/controllers/projects/deployments_controller.rb
index 32111b07a0b..eaf2627677f 100644
--- a/app/controllers/projects/deployments_controller.rb
+++ b/app/controllers/projects/deployments_controller.rb
@@ -49,7 +49,7 @@ class Projects::DeploymentsController < Projects::ApplicationController
 
   # rubocop: disable CodeReuse/ActiveRecord
   def deployment
-    @deployment ||= environment.deployments.find_by(iid: params[:id])
+    @deployment ||= environment.deployments.success.find_by(iid: params[:id])
   end
   # rubocop: enable CodeReuse/ActiveRecord

added 1 commit

62e10d2f - Add API for manually creating deployments

Compare with previous version

added 1 commit

32569c55 - Add API for manually creating deployments

Compare with previous version

added 1 commit

caf4a644 - Add API for manually creating deployments

Compare with previous version

mentioned in issue #33674 (closed)

mentioned in merge request !18456 (closed)

changed the description

added 818 commits

caf4a644...3d8b0ab7 - 817 commits from branch master
d01d3d15 - Add API for manually creating deployments

Compare with previous version

added 1 commit

1d0ce9f7 - Add API for manually creating deployments

Compare with previous version

resolved all threads

added 1 commit

3d94fb6f - Add API for manually creating deployments

Compare with previous version

added 135 commits

3d94fb6f...b70abe63 - 134 commits from branch master
d732e382 - Add API for manually creating deployments

Compare with previous version

@yorickpeterse I've left 2 minor comments.

It LGTM

mentioned in issue gitlab-com/www-gitlab-com#5356 (closed)

Regarding the scope redefinition (from success to visible), we'd want to retain the following behavior in addition.

EnvironmentsFinder
Deployment#previous_deployment
Environment.deployed_to_cluster
Gitlab::CycleAnalytics::Summary::Deploy#value

diff --git a/app/finders/environments_finder.rb b/app/finders/environments_finder.rb
index d4e803beb4e..878e259028c 100644
--- a/app/finders/environments_finder.rb
+++ b/app/finders/environments_finder.rb
@@ -9,7 +9,7 @@ class EnvironmentsFinder
 
   # rubocop: disable CodeReuse/ActiveRecord
   def execute
-    deployments = project.deployments
+    deployments = project.deployments.success
     deployments =
       if ref
         deployments_query = params[:with_tags] ? 'ref = :ref OR tag IS TRUE' : 'ref = :ref'
diff --git a/app/models/deployment.rb b/app/models/deployment.rb
index 30694313f7a..e766d4b8778 100644
--- a/app/models/deployment.rb
+++ b/app/models/deployment.rb
@@ -135,7 +135,7 @@ class Deployment < ApplicationRecord
 
   def previous_deployment
     @previous_deployment ||=
-      project.deployments.joins(:environment)
+      project.deployments.success.joins(:environment)
       .where(environments: { name: self.environment.name }, ref: self.ref)
       .where.not(id: self.id)
       .take
diff --git a/app/models/environment.rb b/app/models/environment.rb
index fe438b142b2..edbadad502c 100644
--- a/app/models/environment.rb
+++ b/app/models/environment.rb
@@ -7,6 +7,7 @@ class Environment < ApplicationRecord
   belongs_to :project, required: true
 
   has_many :deployments, -> { success }, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
+  has_many :successful_deployments, -> { success }, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
 
   has_one :last_deployment, -> { success.order('deployments.id DESC') }, class_name: 'Deployment'
 
diff --git a/ee/app/models/ee/environment.rb b/ee/app/models/ee/environment.rb
index 98da2c2cea0..f8b456036fd 100644
--- a/ee/app/models/ee/environment.rb
+++ b/ee/app/models/ee/environment.rb
@@ -25,7 +25,7 @@ module EE
           .on(join_conditions)
 
         model
-          .joins(:deployments)
+          .joins(:successful_deployments)
           .joins(join.join_sources)
           .where(later_deployments[:id].eq(nil))
           .where(deployments[:cluster_id].eq(cluster.id))
diff --git a/lib/gitlab/cycle_analytics/summary/deploy.rb b/lib/gitlab/cycle_analytics/summary/deploy.rb
index 0691f3cd131..5ff8d881143 100644
--- a/lib/gitlab/cycle_analytics/summary/deploy.rb
+++ b/lib/gitlab/cycle_analytics/summary/deploy.rb
@@ -12,7 +12,7 @@ module Gitlab
 
         def value
           strong_memoize(:value) do
-            query = @project.deployments.where("created_at >= ?", @from)
+            query = @project.deployments.success.where("created_at >= ?", @from)
             query = query.where("created_at <= ?", @to) if @to
             query.count
           end

I assume these code still assume that deployments are only successful ones. I'm not sure if this MR is the right timing to switch the state to visible. Maybe it's fine, maybe it's not. If we want to go safe, I'd recommend to unchange these queries.

added 163 commits

d732e382...c7c4ac07 - 162 commits from branch master
ec50a963 - Add API for manually creating deployments

Compare with previous version

approved this merge request

resolved all threads

added 1 commit

77831f78 - Add API for manually creating deployments

Compare with previous version

All feedback has been addressed since the last comments from @nolith and @dosuken123. I will self-merge this MR since @nolith is in a meeting, and @mayra-cabrera has limited availability.

merged

mentioned in commit b5698d89

This merge request has been deployed to the GitLab.com environment gstg in GitLab auto-deploy version 12.5.201910181441-2840e55ce81.2c3d988bc27.

A list of all the deployed commits can be found here.

If this message is incorrect, please create an issue in the Release Tools project.

added workflowstaging label

This merge request has been deployed to the GitLab.com environment gprd-cny in GitLab auto-deploy version 12.5.201910181611-0e052976891.714fe27b068.

A list of all the deployed commits can be found here.

If this message is incorrect, please create an issue in the Release Tools project.

added workflowcanary label and removed workflowstaging label

mentioned in merge request !18827 (closed)

I hope that we introduce detailed_status for that in the future :)

This merge request has been deployed to the GitLab.com environment gprd in GitLab auto-deploy version 12.5.201910210010-d92a13b6173.bef851d243d.

A list of all the deployed commits can be found here.

If this message is incorrect, please create an issue in the Release Tools project.

added workflowproduction label and removed workflowcanary label

mentioned in issue #34697 (closed)

mentioned in issue #34753 (closed)

@yorickpeterse While this change makes sense from a logical point of view, this did break the current Deployment API behavior which was to only return successful deployments. This in turn broke our automatic Review Apps cleanup: #34697 (closed). @kwiebers suggested that this MR broke the previous API behavior, and indeed, that's what I found out too in #34753 (comment 234274348).

If we don't want to revert this MR, I think that would be great to add a way to filter deployments by status for this API endpoint, so that the previous behavior can be used.

@rymai Reverting is a bit of an overkill. For review apps we would just need to change any code like deployments.whatever to deployments.success.whatever. I see the issue is already closed, has it been solved in the mean time?

Always using success here would break other parts of the API that want to operate on all deployments.

For review apps we would just need to change any code like deployments.whatever to deployments.success.whatever.

@yorickpeterse Yeah, that's what I do in: !18992 (merged)

Always using success here would break other parts of the API that want to operate on all deployments.

This is still kind of a breaking change for people relying on the previous behavior of the API endpoint and I wouldn't be surprised if people reported this change as such. We'll see...

@rymai It's a change that we need for our own release process. Sadly there isn't really a way around this I think. Delaying this work to the future means delaying everything that depends on it, which I believe is not worth it. There is also no gradual way that makes sense, either the data is there or it isn't

@yorickpeterse No worries, the API behavior change isn't the end of the world since the endpoint "only" returns way more data than before, but it's sad that, as an API consumer, there's no way to have the same results as before directly from the API call (i.e. not having to iterate over all results and filter them manually based on their status) , e.g. with a status parameter that would default to success by default so that the previous behavior would be kept, and that could be set to all to have the new behavior.

@yorickpeterse I'll second to @rymai's point about:

If we don't want to revert this MR, I think that would be great to add a way to filter deployments by status for this API endpoint, so that the previous behavior can be used.

Since this is changing the behaviour of GET /projects/:id/deployments, I think we should update that API, adding another option about "deployments status", and by default use success, and pass all to get all deployments as described in:

e.g. with a status parameter that would default to success by default so that the previous behavior would be kept, and that could be set to all to have the new behavior.

This way, we don't break any API, yet we're adding another API feature which we need here. I am not sure if we intended to break the API compatibility? We didn't mention this in a changelog entry either, so I assume we don't intend that.

Sorry @rymai, I am reviewing !18992 (merged) now.

@godfat @rymai Adding an option like that would have helped in this case. However, I think it's a bit odd you have to explicitly opt in to see all your deployments. I know that is what the old behaviour was, but I think it was weird to begin with that we only displayed successful deployments.

Looking at https://dashboards.gitlab.net/d/000000126/grape-endpoints?orgId=1&var-action=Grape%23GET%20%2Fapi%2Fprojects%2F:id%2Fdeployments&var-database=influxdb-01-inf-gprd&from=now-7d&to=now&fullscreen&edit&panelId=1 we seem to get quite a few requests per day to this API, but it's not clear how much of that is our own traffic, and how much of these requests would be affected by this change.

If we think this is important enough we can definitely add the option, though we might end up again breaking things that now depend on this new behaviour. If we are the only ones that run into this, I think it might be easier to change the code that depends on this (at least we wouldn't have to go through the usual release process).

However, I think it's a bit odd you have to explicitly opt in to see all your deployments. I know that is what the old behaviour was, but I think it was weird to begin with that we only displayed successful deployments.

@yorickpeterse I see your point here, since the API is GET /projects/:id/deployments. Taking the old behaviour aside, this endpoint alone implies that it returns all the deployments belonging to the project.

Nonetheless, we are now in a situation where there is already an old behaviour that returned only success deployments. From the point of view of API consumers that have been built on the old behaviour, the API now returns more deployments than what the consumers expected.

In the case of !18992 (merged), we are able to sidestep having to go through all the pages of deployments, as we are interested only in recent deployments. However, there may be other existing consumers that still need to process all the success deployments in history. Having both success and non-success deployments in the list would require those consumers to page through every single deployments. This also possibly creates additional load to GitLab.

I think the middleground we can reach here is to add an option to filter the deployment by the status, while the default behaviour is still returning all deployments. This way, any consumer that is only interested in success can make a small change that would enable them to return to the old behaviour.

GET /projects/:id/deployments # returns all deployments
GET /projects/:id/deployments?status=requested_status # returns all deployments matching requested status

@caalberts That sounds reasonable. I have created #34863 (closed) to track this.

mentioned in merge request !18956 (merged)

mentioned in issue #34863 (closed)

mentioned in commit Dalahro/gitlab@4d1a3d31

mentioned in merge request !46614 (merged)

mentioned in merge request !133805 (merged)

Add API for manually creating deployments

What does this MR do?

UI changes

TODO

Does this MR meet the acceptance criteria?

Conformity

Activity

Reviewer roulette

Add API for manually creating deployments

What does this MR do?

UI changes

TODO

Does this MR meet the acceptance criteria?

Conformity

Merge request reports

Activity

Reviewer roulette