Include and process visibility_level
What does this MR do and why?
This change removes the omission of the visibility_level
attribute from the file-based group export file, and adds logic to assign a visibility_level
to imported subgroups which correctly aligns with those permitted by the destination group's visibility_level
.
This change is needed because previously, with the visibility_level
attribute omitted from the export file, the visibility level for each top-level source group would be set to the same visibility as the destination group, and any source subgroups would be set to PRIVATE, regardless of their original visibility level on the source. See #405168 (closed)
Now, the source subgroup visibility_level
is maintained when it conforms to the rules for subgroup visibility levels (public
groups can contain public, internal or private subgroups, internal
groups can contain internal or private subgroups, private
groups can only contain private subgroups), or is changed to the same visibility level of the destination group.
Changelog: fixed
Screenshots or screen recordings
Screenshots are required for UI changes, and strongly recommended for all other merge requests.
With this public source group:

Imported to a public destination group
The source visibility levels are maintained for each subgroup:

Imported to an internal destination group
The allowed visibility levels are maintained for each subgroup, with the public
subgroup becoming internal
:

Imported to a private destination group
All subgroup visibility levels become private
:

How to set up and validate locally
- On a source instance create a public group with a public, an internal, and a private subgroup.
- In the Group's Settings > General > Advanced > Export group, click the [Export Group] button
- refresh the page until the [Download Export] button appears and download the export file.
- on the destination instance create a public, internal, and private group and take note of their IIds
- In a terminal window, in the same directory as the downloaded export file, make a curl request to the destination instance to create several POST requests, using the id of each destination group just created:
curl --request POST --header "PRIVATE-TOKEN: [ACCESS TOKEN FOR DESTINATION INSTANCE]" \
--form "name=[NAME FOR IMPORTED GROUP]" --form "path=[PATH FOR IMPORTED GROUP]" --form "parent_id=[DESTINATION GROUP ID]" \
--form "file=@[NAME OF EXPORT FILE eg:public-group-3_export.tar.gz]" "[DESTINATION INSTANCE API ENDPOINT eg:]http://gdk.test:3000/api/v4/groups/import"
For each POST request you should see the source group and subgroups imported with the correct visibility level.
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.
Related to #405168 (closed)
Merge request reports
Activity
changed milestone to %16.1
assigned to @carlad-gl
1 Warning This merge request is quite big (645 lines changed), please consider splitting it into multiple merge requests. Reviewer 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 Małgorzata Ksionek (
@mksionek
) (UTC+2, same timezone as@carlad-gl
)Matthias Käppler (
@mkaeppler
) (UTC+2, same timezone as@carlad-gl
)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
Dangeradded workflowin review label and removed workflowready for development label
added bugfunctional label
- Resolved by George Koltsov
Hey @rodrigo.tomonari are you able to give this an initial backend review?
In terms of functionality it all works as expected, but I noticed while validating locally that validation ERRORs get raised during import due to incompatible visibility levels:
"Validation failed: Visibility level public is not allowed since the parent group has a internal visibility."
What seems to be happening is that when processing the root Group, the
Group::GroupRestorer
is used to restore the group, and this somehow tries to create all the subgroups too.Then the subgroups are processed, and this assigns a valid
visibility_level
and they get created/saved.I'm not sure how to fix this as the validation is happening on the model.
Sample backtrace:
{"severity":"ERROR","time":"2023-06-06T09:42:40.690Z","correlation_id":"01H2819YMX3A6QHYZJ85FHX3EX","meta.caller_id":"GroupImportWorker","meta.remote_ip":"172.16.123.1","meta.feature_category":"importers","meta.user":"root","meta.user_id":1,"meta.client_id":"user/1","meta.root_caller_id":"POST /api/:version/groups/import","exception.class":"ActiveRecord::RecordInvalid","exception.message":"Validation failed: Visibility level public is not allowed since the parent group has a internal visibility.","exception.backtrace":["lib/gitlab/import_export/group/relation_tree_restorer.rb:138:in `update_params!'","lib/gitlab/import_export/group/relation_tree_restorer.rb:34:in `block (2 levels) in restore'","lib/gitlab/import_export/group/relation_tree_restorer.rb:33:in `block in restore'","lib/gitlab/database.rb:241:in `block (3 levels) in all_uncached'","lib/gitlab/database.rb:241:in `block (3 levels) in all_uncached'","lib/gitlab/database.rb:241:in `block in all_uncached'","lib/gitlab/database.rb:239:in `all_uncached'","lib/gitlab/import_export/group/relation_tree_restorer.rb:32:in `restore'","lib/gitlab/import_export/group/group_restorer.rb:33:in `restore'","lib/gitlab/import_export/group/tree_restorer.rb:117:in `restore_group'","lib/gitlab/import_export/group/tree_restorer.rb:76:in `process_child'","lib/gitlab/import_export/group/tree_restorer.rb:25:in `block in restore'","lib/gitlab/import_export/group/tree_restorer.rb:24:in `each'","lib/gitlab/import_export/group/tree_restorer.rb:24:in `restore'","app/services/groups/import_export/import_service.rb:32:in `all?'","app/services/groups/import_export/import_service.rb:32:in `execute'","app/workers/group_import_worker.rb:19:in ...[FILTERED]"],"class":"GroupImportWorker","jid":"9373fd690aef7be91fc06fb5","created_at":1686044474.6217659,"correlation_id":"01H2819YMX3A6QHYZJ85FHX3EX","meta.caller_id":"POST /api/:version/groups/import","meta.remote_ip":"172.16.123.1","meta.feature_category":"importers","meta.user":"root","meta.user_id":1,"meta.client_id":"user/1","meta.root_caller_id":"POST /api/:version/groups/import","worker_data_consistency":"always","idempotency_key":"resque:gitlab:duplicate:default:4f0a4e4de1650190d74d9bebecd3749e09618a2e85dee73fbb1a29cf8821f979","size_limiter":"validated","enqueued_at":1686044474.6233559},"extra.importer":"Import/Export","extra.group_id":1771,"extra.group_name":"public-import-010","extra.group_path":"debug-405168-internal/public-import-010"}
requested review from @rodrigo.tomonari
- Resolved by Petar Prokić
@pprokic do you have a moment to go through the validation steps to confirm this fix for #405168 (closed) works as expected?
Allure report
allure-report-publisher
generated test report!e2e-test-on-gdk:
test report for 0e96d3dbexpand test summary
+-----------------------------------------------------------------------+ | suites summary | +------------------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +------------------+--------+--------+---------+-------+-------+--------+ | Monitor | 4 | 0 | 0 | 0 | 4 | ✅ | | Plan | 4 | 0 | 0 | 1 | 4 | ❗ | | Create | 8 | 0 | 1 | 1 | 9 | ❗ | | Govern | 2 | 0 | 0 | 0 | 2 | ✅ | | Manage | 1 | 0 | 0 | 0 | 1 | ✅ | | Framework sanity | 0 | 0 | 1 | 0 | 1 | ➖ | | Data Stores | 2 | 0 | 0 | 1 | 2 | ❗ | +------------------+--------+--------+---------+-------+-------+--------+ | Total | 21 | 0 | 2 | 3 | 23 | ❗ | +------------------+--------+--------+---------+-------+-------+--------+
e2e-package-and-test:
test report for 0e96d3dbexpand test summary
+-----------------------------------------------------------------------+ | suites summary | +------------------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +------------------+--------+--------+---------+-------+-------+--------+ | Create | 688 | 0 | 117 | 5 | 805 | ❗ | | Fulfillment | 12 | 0 | 125 | 0 | 137 | ✅ | | Govern | 245 | 0 | 10 | 5 | 255 | ❗ | | Package | 268 | 0 | 49 | 228 | 317 | ❗ | | Manage | 207 | 18 | 17 | 29 | 242 | ❌ | | Configure | 1 | 0 | 15 | 0 | 16 | ✅ | | Plan | 395 | 0 | 3 | 0 | 398 | ✅ | | Verify | 245 | 0 | 25 | 0 | 270 | ✅ | | Monitor | 52 | 0 | 11 | 0 | 63 | ✅ | | Release | 30 | 0 | 0 | 0 | 30 | ✅ | | ModelOps | 0 | 0 | 5 | 0 | 5 | ➖ | | Systems | 15 | 0 | 0 | 0 | 15 | ✅ | | Data Stores | 185 | 0 | 3 | 0 | 188 | ✅ | | Analytics | 11 | 0 | 0 | 0 | 11 | ✅ | | Framework sanity | 0 | 0 | 7 | 0 | 7 | ➖ | | Secure | 10 | 0 | 45 | 0 | 55 | ✅ | | Growth | 0 | 0 | 10 | 0 | 10 | ➖ | | GitLab Metrics | 2 | 0 | 1 | 0 | 3 | ✅ | +------------------+--------+--------+---------+-------+-------+--------+ | Total | 2366 | 18 | 443 | 267 | 2827 | ❌ | +------------------+--------+--------+---------+-------+-------+--------+
@pprokic
, thanks for approving this merge request.This is the first time the merge request is approved. To ensure full test coverage, a new pipeline will be started shortly.
For more info, please refer to the following links:
added pipeline:mr-approved label
- Resolved by Rodrigo Tomonari
- Resolved by Rodrigo Tomonari
- Resolved by Rodrigo Tomonari
mentioned in issue #405168 (closed)
- Resolved by Rodrigo Tomonari
added 1 commit
- 37328f69 - Move subgroup visibility check into process_root
added 1 commit
- e7fbd630 - Move subgroup visibility check into process_root
added 1954 commits
-
c58b3752...7b63c110 - 1946 commits from branch
master
- 40aaed1b - Include and process visibility_level
- ede9d7ab - Ensure visibility is closest allowed
- 42301ad9 - Use parent of parent as destination group
- 99f0c9f0 - Add spec for nested group import
- 80e1d8eb - Assign visiblity level when processing root
- 3ba98946 - Move subgroup visibility check into process_root
- 47923139 - Add check for restricted visibility levels
- e612ed5a - Fix breaking spec and undercoverage
Toggle commit list-
c58b3752...7b63c110 - 1946 commits from branch
- Resolved by Carla Drago
requested review from @georgekoltsov
requested review from @bmarjanovic and removed review request for @georgekoltsov
- Resolved by George Koltsov
- Resolved by Carla Drago
- Resolved by Carla Drago
- Resolved by Carla Drago
- Resolved by Carla Drago
requested review from @georgekoltsov and removed review request for @bmarjanovic
- Resolved by George Koltsov
@carlad-gl Some end-to-end (E2E) tests have been selected based on the stage label on this MR. If not run already, please run thee2e:package-and-test-ee
job in theqa
stage and review the results before merging this MR. (E2E tests are not run automatically on some MRs due to runner resource constraints.)If you would like to run all e2e tests, please apply the pipeline:run-all-e2e label and restart the pipeline.
Once done, please apply the
emoji on this comment.For any questions or help in reviewing the E2E test results, please reach out on the internal #quality Slack channel.
added pipeline:run-all-e2e label
Thanks @carlad-gl LGTM
Tested it locally via UI and it works as expected
Will wait for pipeline to finish (including the e2e tests) and will proceed with merging.enabled an automatic merge when the pipeline for d3f2c2f2 succeeds