Skip to content
Snippets Groups Projects

Include and process visibility_level

Merged Carla Drago requested to merge 405168-fix-group-visibility-level into master

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

  1. On a source instance create a public group with a public, an internal, and a private subgroup.
  2. In the Group's Settings > General > Advanced > Export group, click the [Export Group] button
  3. refresh the page until the [Download Export] button appears and download the export file.
  4. on the destination instance create a public, internal, and private group and take note of their IIds
  5. 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.

Related to #405168 (closed)

Edited by Carla Drago

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Carla Drago changed milestone to %16.1

    changed milestone to %16.1

  • assigned to @carlad-gl

  • Contributor
    1 Warning
    :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 current availability (@mksionek) (UTC+2, same timezone as @carlad-gl) Matthias Käppler current availability (@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 :repeat: danger-review job that generated this comment.

    Generated by :no_entry_sign: Danger

  • Carla Drago changed the description

    changed the description

  • Carla Drago added 1 commit

    added 1 commit

    • 6f671886 - Include and process visibility_level

    Compare with previous version

    • Author Maintainer
      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

  • Contributor

    Allure report

    allure-report-publisher generated test report!

    e2e-test-on-gdk: :exclamation: test report for 0e96d3db

    expand 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: :x: test report for 0e96d3db

    expand 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  | ❌     |
    +------------------+--------+--------+---------+-------+-------+--------+
  • Petar Prokić approved this merge request

    approved this merge request

  • :wave: @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:

  • Rodrigo Tomonari
  • Carla Drago added 1 commit

    added 1 commit

    • dc284e3e - Ensure visibility is closest allowed

    Compare with previous version

  • Carla Drago added 1 commit

    added 1 commit

    • 503e5ee0 - Use parent of parent as destination group

    Compare with previous version

  • Carla Drago added 1 commit

    added 1 commit

    • 53060bdf - Add spec for nested group import

    Compare with previous version

  • Carla Drago added 1 commit

    added 1 commit

    • c7b6fb90 - Assign visiblity level when processing root

    Compare with previous version

  • Carla Drago added 1 commit

    added 1 commit

    • 54ace4ff - Assign visiblity level when processing root

    Compare with previous version

  • Carla Drago added 1 commit

    added 1 commit

    • 44fb0384 - Assign visiblity level when processing root

    Compare with previous version

  • mentioned in issue #405168 (closed)

  • Rodrigo Tomonari
  • Carla Drago added 1 commit

    added 1 commit

    • 37328f69 - Move subgroup visibility check into process_root

    Compare with previous version

  • Carla Drago added 1 commit

    added 1 commit

    • e7fbd630 - Move subgroup visibility check into process_root

    Compare with previous version

  • Carla Drago added 1 commit

    added 1 commit

    • 5b3dc697 - Add check for restricted visibility levels

    Compare with previous version

  • Carla Drago added 1 commit

    added 1 commit

    Compare with previous version

  • Carla Drago added 1954 commits

    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

    Compare with previous version

  • Rodrigo Tomonari approved this merge request

    approved this merge request

  • Carla Drago added 1 commit

    added 1 commit

    Compare with previous version

  • Carla Drago requested review from @georgekoltsov

    requested review from @georgekoltsov

  • Rodrigo Tomonari approved this merge request

    approved this merge request

  • Carla Drago requested review from @bmarjanovic and removed review request for @georgekoltsov

    requested review from @bmarjanovic and removed review request for @georgekoltsov

  • George Koltsov
  • Bojan Marjanovic
  • Bojan Marjanovic
  • Carla Drago requested review from @georgekoltsov and removed review request for @bmarjanovic

    requested review from @georgekoltsov and removed review request for @bmarjanovic

  • Carla Drago added 1 commit

    added 1 commit

    • 0e96d3db - Add specs for nested subgroups

    Compare with previous version

    • Contributor
      Resolved by George Koltsov

      :warning: @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 the e2e:package-and-test-ee job in the qa 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 :white_check_mark: emoji on this comment.

      For any questions or help in reviewing the E2E test results, please reach out on the internal #quality Slack channel.

  • Thanks @carlad-gl LGTM :rocket:

    Tested it locally via UI and it works as expected :tada: Will wait for pipeline to finish (including the e2e tests) and will proceed with merging.

  • George Koltsov approved this merge request

    approved this merge request

  • George Koltsov resolved all threads

    resolved all threads

  • George Koltsov enabled an automatic merge when the pipeline for d3f2c2f2 succeeds

    enabled an automatic merge when the pipeline for d3f2c2f2 succeeds

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading