Fix deployment approval popup to support multiple approval rules
What does this MR do and why?
This merge request ensures the deployment approval popup supports multiple approval rules in environments index/detail pages.
It builds on the fix provided by @shinya.maeda in his proof of concept merge request, while taking into consideration the following:
- The
required_approval_count
method ofEE::Environment
model is in fact used for determining if the popup should be displayed given a unified approval setting. - This can be seen in how
pending_approval_count
which uses the former method internally is used in views likedeployment.vue
andenvironment_approval.vue
. - So in order to ensure we have that logic in one single place, I opted for updating
required_approval_count
to consider multiple approval rules. - With that in mind, I have also decided to update
needs_approval?
method in the same class to depend only onrequired_approval_count
as we already check for existence of approval rules there viahas_approval_rules?
method.
Additionally, in order to make sure we show the popup in environment detail page, I had to do move the logic in show_deployment_approval?
method in EE::EnvironmentsHelper
to be in a new method can_approve_deployment?
and adjust it to also consider multiple approval rules. This new method is also used in the aforementiond views to determine if a user can approve deployments based on their access and whether there exist an approval rule for a group or access level to which this user belong.
Resolves #362237 (closed).
Screenshots or screen recordings
Unified approval setting
In the screenshots below, the staging
environment is protected and has unified approval setting configured via UI (where required_approval_count
= 1).
This worked fine before (and still works fine after) the fix introduced in this merge request:
Multiple approval rules
In the screenshows below, the staging
environment is also used, and is protected. This time, however, multiple approval rules are configured via the API (explained below) and there are two approval rules for different groups, each with its own required_approvals
= 1 (so the sum of required approvals is 2; please note the current approvals
count inside the popup).
The popup was not showing before the fix introduced in this merge request:
How to set up and validate locally
To validate locally, please follow the steps below:
- Make sure your local GDK setup is configured to work on GitLab EE (since both Protected Environments and Deployment Approvals are Premium features).
- Create a new project or use an existing one.
- Create a new group or use an existing one.
- In your project, create a new environment from Deployments > Environments, and let's name it
staging
. - There's currently no way to configure multiple approval rules via UI, therefore, you need to configure them for this environment via REST API:
curl --header 'Content-Type: application/json' \
--header "PRIVATE-TOKEN: YOUR_TOKEN" \
--request POST \
--data '{"name": "staging", "deploy_access_levels": [{"access_level": 40}], "approval_rules": [{"group_id": GROUP_ID}]}' \
"http://gdk.test:3000/api/v4/projects/PROJECT_ID/protected_environments"
Please remember to change GROUP_ID
, PROJECT_ID
, YOUR_TOKEN
with the respective values and the token you wish to use.
- From Settings > CI/CD > Protected Environments, verify that
staging
is now protected. - In your project, update
.gitlab-ci.yml
file to include a deployment step:
deploy_staging:
stage: deploy
script:
- echo "Deploy to staging"
environment:
name: staging
- Push a commit to the project. A new deployment will be created.
- Verify that this deployment is blocked waiting for approval by going to Deployments > Environments.
- You should see the deployment approval popup under
staging
environment as shown in the screenshows above.
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.
Merge request reports
Activity
changed milestone to %15.2
assigned to @ahmed.hemdan
Suggested Reviewers (beta)
The individuals below may be good candidates to participate in the review based on various factors.
You can use slash commands in comments to quickly assign
/assign_reviewer @user1
.Suggested Reviewers @tkuah
,@rspeicher
,@mayra-cabrera
,@kushalpandya
,@dzaporozhets
If you do not believe these suggestions are useful, please apply the label Bad Suggested Reviewer. You can also provide feedback for this feature on this issue:
https://gitlab.com/gitlab-org/gitlab/-/issues/357923
.Automatically generated by Suggested Reviewers Bot - an experimental ML-based recommendation engine created by ~"group::applied ml".
Edited by GitLab Reviewer-Recommender BotReviewer roulette
Changes that require review have been detected!
Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:
Category Reviewer Maintainer backend Niko Belokolodov ( @nbelokolodov
) (UTC+8, 6 hours ahead of@ahmed.hemdan
)Heinrich Lee Yu ( @engwan
) (UTC+8, 6 hours ahead of@ahmed.hemdan
)UX Nick Leonard ( @nickleonard
) (UTC-5, 7 hours behind@ahmed.hemdan
)Jeremy Elder ( @jeldergl
) (UTC-5, 7 hours behind@ahmed.hemdan
)To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.
To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.
Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.
If needed, you can retry the
danger-review
job that generated this comment.Generated by
DangerAllure report
allure-report-publisher
generated test report!review-qa-blocking:
test report for 232a535eexpand test summary
+---------------------------------------------------------------------------+ | suites summary | +----------------------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +----------------------+--------+--------+---------+-------+-------+--------+ | Create | 23 | 0 | 2 | 23 | 25 | ❗ | | Plan | 47 | 0 | 1 | 47 | 48 | ❗ | | Manage | 37 | 0 | 2 | 39 | 39 | ❗ | | Verify | 12 | 0 | 1 | 12 | 13 | ❗ | | Secure | 2 | 0 | 0 | 2 | 2 | ❗ | | Package | 0 | 0 | 1 | 0 | 1 | ➖ | | Protect | 2 | 0 | 0 | 2 | 2 | ❗ | | Version sanity check | 0 | 0 | 1 | 0 | 1 | ➖ | | Configure | 0 | 0 | 1 | 0 | 1 | ➖ | +----------------------+--------+--------+---------+-------+-------+--------+ | Total | 123 | 0 | 9 | 125 | 132 | ❗ | +----------------------+--------+--------+---------+-------+-------+--------+
requested review from @vshushlin
- Resolved by Shinya Maeda
@vshushlin I see a couple of failures in the pipeline, but I'm fixing them. Can you please review this as backend maintainer?
requested review from @pedroms
marked the checklist item I have evaluated the MR acceptance checklist for this MR. as completed
mentioned in issue #362237 (closed)
added workflowin review label and removed workflowin dev label
removed review request for @pedroms
@pedroms
, thanks for approving this merge request.This is the first time the merge request is approved. To ensure full test coverage, a new pipeline has been started.
For more info, please refer to the following links:
- Resolved by Ahmed Hemdan
@ahmed.hemdan super sorry, but it takes me too long to review =(
@shinya.maeda can you review it? I would ask you to review this as a maintainer anyway, as I don't really know much about the feature.
requested review from @shinya.maeda and removed review request for @vshushlin
@ahmed.hemdan Sorry for late response. I'll take a look shortly.
- Resolved by Shinya Maeda
@ahmed.hemdan To fix the pipeline failure, please try the following patch:
diff --git a/ee/app/models/ee/environment.rb b/ee/app/models/ee/environment.rb index 47644c1066d..c45baef418b 100644 --- a/ee/app/models/ee/environment.rb +++ b/ee/app/models/ee/environment.rb @@ -87,7 +87,7 @@ def required_approval_count return 0 unless protected? if has_approval_rules? - associated_approval_rules.sum(:required_approvals) + associated_approval_rules.sum(&:required_approvals) else associated_protected_environments.map(&:required_approval_count).max end @@ -108,7 +108,7 @@ def find_approval_rule_for(user, represented_as: nil) def associated_approval_rules strong_memoize(:associated_approval_rules) do ::ProtectedEnvironments::ApprovalRule - .where(protected_environment: associated_protected_environments) + .where(protected_environment: associated_protected_environments).to_a # Return Array instead of AR relationship, otherwise `associated_approval_rules.any?` and `associated_approval_rules.sum(:required_approvals)` executes a new query end end diff --git a/ee/spec/lib/ee/api/entities/deployment_extended_spec.rb b/ee/spec/lib/ee/api/entities/deployment_extended_spec.rb index b9ed536d605..5f9a3b0042b 100644 --- a/ee/spec/lib/ee/api/entities/deployment_extended_spec.rb +++ b/ee/spec/lib/ee/api/entities/deployment_extended_spec.rb @@ -10,9 +10,10 @@ before do stub_licensed_features(protected_environments: true) - protected_environment = create(:protected_environment, project_id: deployment.environment.project_id, name: deployment.environment.name, required_approval_count: 2) + # To avoid confusion, we test aginst multiple approval rules instead of unified approval setting. + protected_environment = create(:protected_environment, project_id: deployment.environment.project_id, name: deployment.environment.name) create(:deployment_approval, :approved, deployment: deployment) - create(:protected_environment_approval_rule, :maintainer_access, protected_environment: protected_environment, required_approvals: 1) + create(:protected_environment_approval_rule, :maintainer_access, protected_environment: protected_environment, required_approvals: 2) end it 'includes fields from deployment entity' do
What's fixed above are:
-
Environment#associated_approval_rules
was returning AR relation object instead of Array object. This resulted in N+1 since some methods try to execute a new query and ignore thestrong_memoize
. - Calculate the
required_approval_count
from the Array object. - Fixed false-positive test
-
@ahmed.hemdan Aside from that, this change looks nice to me
Please assign me back if the pipeline becomes green or let me know if you have questions.removed review request for @shinya.maeda
added 1 commit
- 39e01986 - Fix N+1 queries in associated_approval_rules
added 1 commit
- 24ef61a7 - Fix N+1 queries in associated_approval_rules
added 1 commit
- 13d54dc4 - Fix N+1 queries in associated_approval_rules
added 1 commit
- e809b8cc - Fix N+1 queries in associated_approval_rules
- Resolved by Jeremy Elder
requested review from @acook.gitlab
- Resolved by Ahmed Hemdan
requested review from @shinya.maeda
removed review request for @acook.gitlab
mentioned in issue gitlab-com/www-gitlab-com#13239 (closed)
added 1 commit
- 232a535e - Update formatting to remove rubocop disable comments
- Resolved by Ahmed Hemdan
mentioned in issue #367410 (closed)
enabled an automatic merge when the pipeline for 09b6e87b succeeds
mentioned in commit dc1cb0d1
added workflowstaging-canary label and removed workflowin review label
added workflowcanary label and removed workflowstaging-canary label
added workflowstaging label and removed workflowcanary label
added workflowproduction label and removed workflowstaging label
added workflowstaging label and removed workflowproduction label
added workflowpost-deploy-db-staging label and removed workflowstaging label
added workflowpost-deploy-db-production label and removed workflowpost-deploy-db-staging label
mentioned in issue #367700 (closed)
added releasedcandidate label
added releasedpublished label and removed releasedcandidate label
mentioned in merge request kubitus-project/kubitus-installer!1252 (merged)
mentioned in merge request !100382 (closed)