Show members of private group as approvers
What does this MR do and why?
Show members of private group as approvers
When an approval rule has a group that is considered hidden to the user then approvers are not shown. This is a problem when a private group has been added to an approval rule.
This change is to consider private groups that are invited to the project associated with the approval rule to be considered non-hidden
For more context: #281029 (comment 1807240087)
Query Plans
This change adds an additional query during project_groups
calling project.invited_groups
that results in a query like:
SELECT *
FROM "namespaces"
INNER JOIN "project_group_links" ON "namespaces"."id" = "project_group_links"."group_id"
WHERE "namespaces"."type" = 'Group'
AND "project_group_links"."project_id" = 278964
query plan here: https://console.postgres.ai/gitlab/gitlab-production-main/sessions/27397/commands/85304
The result of that is used in hidden_groups
which performs a query like:
https://console.postgres.ai/gitlab/gitlab-production-main/sessions/27497/commands/85645
SELECT *
FROM "namespaces"
INNER JOIN "approval_project_rules_groups" ON "namespaces"."id" = "approval_project_rules_groups"."group_id"
WHERE "namespaces"."type" = 'Group'
AND "approval_project_rules_groups"."approval_project_rule_id" = 14315753
AND "namespaces"."id" NOT IN (2750817,
3887968,
5924764,
6150316);
Before this change hidden_groups
would perform a similar query but without the results from project_groups
:
https://console.postgres.ai/gitlab/gitlab-production-main/sessions/27497/commands/85648
SELECT *
FROM "namespaces"
INNER JOIN "approval_project_rules_groups" ON "namespaces"."id" = "approval_project_rules_groups"."group_id"
WHERE "namespaces"."type" = 'Group'
AND "approval_project_rules_groups"."approval_project_rule_id" = 14315753
AND "namespaces"."id" != 3887968;
Notably sometimes rule
is an instance of ApprovalMergeRequestRule
, but the resulting queries are similar
For example
SELECT *
FROM "namespaces"
INNER JOIN "approval_merge_request_rules_groups" ON "namespaces"."id" = "approval_merge_request_rules_groups"."group_id"
WHERE "namespaces"."type" = 'Group'
AND "approval_merge_request_rules_groups"."approval_merge_request_rule_id" = 269
AND "namespaces"."id" != 2962
Hash Join (cost=7.54..14.83 rows=1 width=434) (actual time=0.278..0.338 rows=1 loops=1)
Hash Cond: (namespaces.id = approval_merge_request_rules_groups.group_id)
-> Index Scan using index_namespaces_on_organization_id_for_groups on namespaces (cost=0.14..7.14 rows=114 width=414) (actual time=0.091..0.271 rows=132 loops=1)
Filter: (id <> 2962)
Rows Removed by Filter: 1
-> Hash (cost=7.29..7.29 rows=8 width=20) (actual time=0.039..0.039 rows=2 loops=1)
Buckets: 1024 Batches: 1 Memory Usage: 9kB
-> Index Scan using index_approval_merge_request_rules_groups_1 on approval_merge_request_rules_groups (cost=0.15..7.29 rows=8 width=20) (actual time=0.029..0.031 rows=2 loops=1)
Index Cond: (approval_merge_request_rule_id = 269)
Planning Time: 0.955 ms
Execution Time: 0.413 ms
MR acceptance checklist
Please evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.
How to set up and validate locally
- Create a private group (
private-group-b
) - Create a project
- Invite the private subgroup to the project
- Add a
CODEOWNERS
file to the project. For example
*.txt @private-group-b
- Create a protected branch and require code owner approval
- Create merge request that changes something requiring code owner approval
- Invite another user to the project
- As the other user, see that the approvers for the code owner rule are missing from the approval rules widget in the merge request
- Enable the
show_private_groups_as_approvers
feature flag - Check the widget again and see that the approvers are present.
Related to #281029 (closed)
Merge request reports
Activity
changed milestone to %16.11
added CISP1 Category:Source Code Management CodeOwners Deliverable SLONear Miss [deprecated] Accepting merge requests approvals backend backend-weight2 bugfunctional customer devopscreate groupsource code missed-deliverable missed:14.3 missed:14.4 priority2 reproduced on GitLab.com sectiondev severity2 typebug workflowin dev labels
assigned to @j.seto
- A deleted user
added database databasereview pending feature flag labels
2 Warnings 5350549a: The commit subject must start with a capital letter. For more information, take a look at our Commit message guidelines. 5350549a: The commit subject and body must be separated by a blank line. For more information, take a look at our Commit message guidelines. Reviewer roulette
Category Reviewer Maintainer backend @shreyasagarwal
(UTC+2, 2 hours ahead of author)
@johnmason
(UTC-4, 4 hours behind author)
database @dgruzd
(UTC+2, 2 hours ahead of author)
@mayra-cabrera
(UTC-6, 6 hours behind author)
Please check reviewer's status!
Please refer to documentation page for guidance on how you can benefit from the Reviewer Roulette, or use the GitLab Review Workload Dashboard to find other available reviewers.
If needed, you can retry the
danger-review
job that generated this comment.Generated by
Danger- Resolved by Mayra Cabrera
requested review from @zmartins
- Resolved by Mayra Cabrera
requested review from @ifarkas
- Resolved by Jerry Seto
removed review request for @zmartins
added pipeline:mr-approved label
- Resolved by Mayra Cabrera
@zmartins
, thanks for approving this merge request.This is the first time the merge request has been approved. To ensure we don't only run predictive pipelines, and we don't break
master
, a new pipeline will be started shortly.Please wait for the pipeline to start before resolving this discussion and set auto-merge for the new pipeline. See merging a merge request for more details.
E2E Test Result Summary
allure-report-publisher
generated test report!e2e-test-on-gdk:
test report for a63ee20eexpand test summary
+------------------------------------------------------------------+ | suites summary | +-------------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +-------------+--------+--------+---------+-------+-------+--------+ | Create | 83 | 0 | 9 | 0 | 92 | ✅ | | Monitor | 7 | 0 | 0 | 0 | 7 | ✅ | | Govern | 66 | 0 | 0 | 0 | 66 | ✅ | | Plan | 51 | 0 | 2 | 0 | 53 | ✅ | | Data Stores | 31 | 0 | 0 | 0 | 31 | ✅ | | Package | 24 | 0 | 6 | 0 | 30 | ✅ | | Verify | 35 | 0 | 1 | 0 | 36 | ✅ | | Release | 5 | 0 | 0 | 0 | 5 | ✅ | | Analytics | 2 | 0 | 0 | 0 | 2 | ✅ | | Manage | 0 | 0 | 1 | 0 | 1 | ➖ | +-------------+--------+--------+---------+-------+-------+--------+ | Total | 304 | 0 | 19 | 0 | 323 | ✅ | +-------------+--------+--------+---------+-------+-------+--------+
e2e-package-and-test:
test report for a63ee20eexpand test summary
+------------------------------------------------------------------+ | suites summary | +-------------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +-------------+--------+--------+---------+-------+-------+--------+ | Create | 570 | 0 | 66 | 10 | 636 | ✅ | | Plan | 8 | 0 | 0 | 0 | 8 | ✅ | | Govern | 6 | 0 | 0 | 0 | 6 | ✅ | | Monitor | 8 | 0 | 0 | 0 | 8 | ✅ | | Data Stores | 4 | 0 | 0 | 0 | 4 | ✅ | | Package | 0 | 0 | 2 | 0 | 2 | ➖ | +-------------+--------+--------+---------+-------+-------+--------+ | Total | 596 | 0 | 68 | 10 | 664 | ✅ | +-------------+--------+--------+---------+-------+-------+--------+
requested review from @splattael
- Resolved by Jerry Seto
removed review request for @splattael
removed review request for @ifarkas
added 467 commits
-
cfe9bbd5...dba7c49a - 464 commits from branch
master
- 0eb7578e - Show members of private group as approvers
- 08351096 - Add some tests
- 5350549a - - Refactor project_groups to private
Toggle commit list-
cfe9bbd5...dba7c49a - 464 commits from branch
reset approvals from @splattael by pushing to the branch
requested review from @ifarkas
added databasereviewed label and removed databasereview pending label
requested review from @mayra-cabrera and removed review request for @ifarkas
- Resolved by Jerry Seto
- Resolved by Jerry Seto
- Resolved by Jerry Seto
The result of that is used in
hidden_groups
which performs a query like:Notably sometimes
rule
is an instance ofApprovalMergeRequestRule
, but the resulting queries are similarNone of these queries return any result. Could we please analyze a query that returns some rows? Just to understand what would be the average query plan. Also, could we use the
gitlab-org
namespace (9970
)?
removed review request for @mayra-cabrera
added 1 commit
- a63ee20e - Return an activerecord collection rather than an Array
reset approvals from @splattael and @ifarkas by pushing to the branch
requested review from @mayra-cabrera
added databaseapproved label and removed databasereviewed label
Thanks @j.seto. database LGTM. I'll proceed to merge since the MR has been approved by a backend maintainer !148720 (comment 1854507434)
enabled an automatic merge when the pipeline for c22c8631 succeeds
Hello @j.seto
The database team is looking for ways to improve the database review process and we would love your help!
If you'd be open to someone on the database team reaching out to you for a chat, or if you'd like to leave some feedback asynchronously, just post a reply to this comment mentioning:
@gitlab-org/database-team
And someone will be by shortly!
Thanks for your help!
This message was generated automatically. You're welcome to improve it.
mentioned in commit 7dc52246
added workflowstaging-canary label and removed workflowin dev label
added workflowcanary label and removed workflowstaging-canary label
added workflowstaging label and removed workflowcanary label
added workflowproduction label and removed workflowstaging label
added workflowpost-deploy-db-production label and removed workflowproduction label
added releasedcandidate label
mentioned in issue #281029 (closed)
added releasedpublished label and removed releasedcandidate label
added pipelinetier-3 label
mentioned in issue gitlab-org/quality/triage-reports#20597 (closed)
mentioned in issue #506056 (closed)
mentioned in issue gitlab-org/quality/triage-reports#20960 (closed)
mentioned in issue gitlab-org/quality/triage-reports#21530 (closed)
mentioned in issue gitlab-org/quality/triage-reports#22034