Add placeholder user mapping to bitbucket server importer
What does this MR do and why?
Mentions https://gitlab.com/gitlab-org/gitlab/-/issues/466356
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.
Screenshots or screen recordings
Screenshots are required for UI changes, and strongly recommended for all other merge requests.
Before | After |
---|---|
How to set up and validate locally
- Set up a Bitbucket Server instance by following these instructions.
- Set up your project and repository on your BBS instance.
- As UserA:
- Create UserB in http://localhost:7990/admin/users and set their password.
- Create a project with a simple README and open an MR with a modification.
- Add UserB to the project and assign UserB as a reviewer.
- Make inline/standalone comments on the MR.
- As UserB:
- Make inline/standalone comments on the MR.
- Approve and merge the MR.
- Make any other contributions on your source instance that you'd like to see imported.
- As UserA:
- Enable BBS import in your GDK and enable the relevant feature flags:
bitbucket_server_user_mapping
&importer_user_mapping
. - Import a project from http://localhost:7990 to the target via BitBucket Server importer.
- On the target instance, go to the target group that you just imported the project into and go to the group members page. Ensure that your source contributions are mapped as expected.
Merge request reports
Activity
assigned to @georgekoltsov
added pipelinetier-1 label
- A deleted user
added backend feature flag groupimport and integrate labels
7 Warnings This merge request is quite big (1749 lines changed), please consider splitting it into multiple merge requests. 3bc1bdc1: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. 40206ef7: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. 85e87983: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. f36256b7: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. 1be8c4d8: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. The master pipeline status page reported failures in If these jobs fail in your merge request with the same errors, then they are not caused by your changes.
Please check for any on-going incidents in the incident issue tracker or in the#master-broken
Slack channel.1 Message CHANGELOG missing: If this merge request needs a changelog entry, add the
Changelog
trailer to the commit message you want to add to the changelog.If this merge request doesn't need a CHANGELOG entry, feel free to ignore this message.
Reviewer roulette
Category Reviewer Maintainer backend @bhrai
(UTC+1, 1 hour ahead of author)
@wandering_person
(UTC+7, 7 hours ahead of author)
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
Dangeradded devopsfoundations sectioncore platform labels
added 1 commit
- 9b6a96fc - Add placeholder user mapping to bitbucket server importer
- Resolved by 🤖 GitLab Bot 🤖
Proper labels assigned to this merge request. Please ignore me.
@georgekoltsov
- please see the following guidance and update this merge request.1 Error Please add typebug typefeature, or typemaintenance label to this merge request. Edited by 🤖 GitLab Bot 🤖
assigned to @jnutt
changed milestone to %17.5
added User Contribution Mapping typefeature labels
added 1 commit
- 34d6a67b - Add placeholder user mapping to bitbucket server [jnutt additions]
- A deleted user
added databasereview pending label
added 1 commit
- 7ed435fe - Add placeholder user mapping to bitbucket server [jnutt additions]
- Resolved by James Nutt
- Resolved by James Nutt
- Resolved by Rodrigo Tomonari
- Resolved by James Nutt
mentioned in merge request !167169 (closed)
- Resolved by James Nutt
- Resolved by Luke Duncalfe
- Resolved by James Nutt
added 1 commit
- 7f692c0f - Add placeholder user mapping to bitbucket server [jnutt additions]
- Resolved by James Nutt
- Resolved by James Nutt
- Resolved by James Nutt
added 1 commit
- 6dfbd84a - Add placeholder user mapping to bitbucket server [jnutt additions]
mentioned in issue #467512 (closed)
added 1 commit
- b0bf7407 - Add placeholder user mapping to bitbucket server [jnutt additions]
added 4817 commits
-
b0bf7407...0a6d6571 - 4815 commits from branch
master
- 70d71e4d - Add placeholder user mapping to bitbucket server importer
- b3badd85 - Add placeholder user mapping to bitbucket server [jnutt additions]
-
b0bf7407...0a6d6571 - 4815 commits from branch
added 1 commit
- 3df5719e - Add placeholder user mapping to bitbucket server [jnutt additions]
added 1 commit
- 7e348197 - Add placeholder user mapping to bitbucket server [jnutt additions]
mentioned in merge request !167078 (merged)
added 1 commit
- b6abb0a2 - Add placeholder user mapping to bitbucket server [jnutt additions]
added 1 commit
- 9322c501 - Add placeholder user mapping to bitbucket server [jnutt additions]
changed milestone to %17.6
added missed:17.5 label
unassigned @georgekoltsov
mentioned in merge request !170084 (merged)
added 4483 commits
-
9322c501...3fdc2624 - 4481 commits from branch
master
- 3324a24e - Add placeholder user mapping to bitbucket server importer
- d6e980fb - Add placeholder user mapping to bitbucket server [jnutt additions]
-
9322c501...3fdc2624 - 4481 commits from branch
added 1 commit
- fd8c98c8 - Add placeholder user mapping to bitbucket server [jnutt additions]
added 1 commit
- cfb159f2 - Add placeholder user mapping to bitbucket server [jnutt additions]
added 1 commit
- 8736e618 - Add placeholder user mapping to bitbucket server [jnutt additions]
added 1 commit
- c85cfb80 - Add placeholder user mapping to bitbucket server [jnutt additions]
added 1 commit
- f0e9d1ea - Add placeholder user mapping to bitbucket server [jnutt additions]
added 1 commit
- 240c3d74 - Add placeholder user mapping to bitbucket server [jnutt additions]
- Resolved by James Nutt
added 1 commit
- 9ff3c1c5 - Add placeholder user mapping to bitbucket server [jnutt additions]
added 1 commit
- eca8c6a2 - Add placeholder user mapping to bitbucket server [jnutt additions]
- Resolved by Rodrigo Tomonari
@SamWord This isn't quite ready (still needs more specs and manual QA), but it's getting large, so could I ask you for an initial review?
requested review from @SamWord
added 1 commit
- 0f326fbf - Add placeholder user mapping to bitbucket server [jnutt additions]
added 1437 commits
-
0f326fbf...d1b64345 - 1435 commits from branch
master
- 18fef561 - Add placeholder user mapping to bitbucket server importer
- 6b79adea - Add placeholder user mapping to bitbucket server [jnutt additions]
-
0f326fbf...d1b64345 - 1435 commits from branch
added 1 commit
- 5aee825d - Add placeholder user mapping to bitbucket server [jnutt additions]
added 1 commit
- 6253ef26 - Add placeholder user mapping to bitbucket server [jnutt additions]
added 1 commit
- 859c30c3 - Add placeholder user mapping to bitbucket server [jnutt additions]
added 1 commit
- 965dff79 - Add placeholder user mapping to bitbucket server [jnutt additions]
mentioned in merge request !171842 (merged)
- Resolved by James Nutt
- Resolved by James Nutt
added 1 commit
- 1ae767d4 - Add placeholder user mapping to bitbucket server [jnutt additions]
added 406 commits
-
1ae767d4...676e1ad2 - 404 commits from branch
master
- 026302e8 - Add placeholder user mapping to bitbucket server importer
- 671cb30e - Add placeholder user mapping to bitbucket server [jnutt additions]
-
1ae767d4...676e1ad2 - 404 commits from branch
added 1 commit
- d74131ef - Add placeholder user mapping to bitbucket server [jnutt additions]
- Resolved by James Nutt
added 1 commit
- 65e372fe - Add placeholder user mapping to bitbucket server [jnutt additions]
added 1 commit
- 701712e2 - Add placeholder user mapping to bitbucket server [jnutt additions]
added 325 commits
-
701712e2...73937faa - 323 commits from branch
master
- 58e09db1 - Add placeholder user mapping to bitbucket server importer
- fa448c19 - Add placeholder user mapping to bitbucket server [jnutt additions]
-
701712e2...73937faa - 323 commits from branch
- Resolved by Sam Word
- Resolved by James Nutt
- Resolved by James Nutt
- Resolved by James Nutt
- Resolved by Sam Word
- Resolved by Sam Word
- Resolved by Sam Word
- Resolved by Sam Word
- Resolved by Sam Word
- Resolved by Sam Word
removed review request for @SamWord
changed milestone to %17.7
added 1402 commits
-
fa448c19...d4ef7906 - 1400 commits from branch
master
- 29a87a7b - Add placeholder user mapping to bitbucket server importer
- cb364027 - Add placeholder user mapping to bitbucket server [jnutt additions]
-
fa448c19...d4ef7906 - 1400 commits from branch
added 1 commit
- f28a380e - Add placeholder user mapping to bitbucket server [jnutt additions]
added 1 commit
- 7b8a3da0 - Add placeholder user mapping to bitbucket server [jnutt additions]
added 1 commit
- 48fefc21 - Add placeholder user mapping to bitbucket server [jnutt additions]
added 1 commit
- c7713782 - Add placeholder user mapping to bitbucket server [jnutt additions]
added 1 commit
- 53064493 - Add placeholder user mapping to bitbucket server [jnutt additions]
added 1 commit
- bdcb3caa - Add placeholder user mapping to bitbucket server [jnutt additions]
added 1 commit
- 6f825e87 - Add placeholder user mapping to bitbucket server [jnutt additions]
requested review from @SamWord
added 1 commit
- d0284f13 - Add placeholder user mapping to bitbucket server [jnutt additions]
added 1 commit
- c55e5b16 - Add placeholder user mapping to bitbucket server [jnutt additions]
removed databasereview pending label
added 1 commit
- 3f109fb5 - Add placeholder user mapping to bitbucket server [jnutt additions]
added 1 commit
- d491c757 - Add placeholder user mapping to bitbucket server [jnutt additions]
added 1 commit
- 4ecdf221 - Add placeholder user mapping to bitbucket server [jnutt additions]
- Resolved by Sam Word
- Resolved by James Nutt
- Resolved by Sam Word
- Resolved by Sam Word
- Resolved by Sam Word
- Resolved by Sam Word
- Resolved by Sam Word
- Resolved by Sam Word
added 1 commit
- 2bcf2ff9 - Add placeholder user mapping to bitbucket server [jnutt additions]
added 1 commit
- d21e5076 - Add placeholder user mapping to bitbucket server [jnutt additions]
- Resolved by James Nutt
added 1 commit
- 116b0cea - Add placeholder user mapping to bitbucket server [jnutt additions]
added 1 commit
- 167b3b06 - Add placeholder user mapping to bitbucket server [jnutt additions]
- Resolved by Sam Word
- Resolved by Sam Word
- Resolved by Sam Word
- Resolved by James Nutt
requested review from @SamWord
- Resolved by Sam Word
- Resolved by James Nutt
- Resolved by Sam Word
- Resolved by James Nutt
- Resolved by Sam Word
- Resolved by James Nutt
- Resolved by James Nutt
- Resolved by Sam Word
- Resolved by James Nutt
- Resolved by Sam Word
- Resolved by James Nutt
- Resolved by Sam Word
- Resolved by James Nutt
- Resolved by James Nutt
- Resolved by James Nutt
- Resolved by James Nutt
- Resolved by James Nutt
- Resolved by Rodrigo Tomonari
- Resolved by James Nutt
- Resolved by Sam Word
- Resolved by Sam Word
- Resolved by Sam Word
- Resolved by Sam Word
- Resolved by James Nutt
- Resolved by James Nutt
added 1 commit
- 81675348 - Add ID to user representations as unique-er ID
added 1 commit
- f33ed1fb - Add ID to user representations as unique-er ID
mentioned in issue #506719 (closed)
added 1 commit
- 200a8e97 - Add ID to user representations as unique-er ID
- Resolved by Rodrigo Tomonari
@jnutt The static code looks good! I worked on some local testing though, and it looks pretty good too, but I noticed there was a placeholder user I didn't add.
It turns out I made a bitbucket server for myself a while ago, so I had an open pull request for some time. I guess after 21 weeks Bitbucket server just declines an open PR, but does so with a system user that gets imported as a placeholder
Should those users be handled differently? It's like some kind of system user that doesn't make sense to map to a real user IMO.
A couple of other minor comments:
- I had to rebase these changes with the other bitbucket server changes on
master
in order to actually reassign users. Without it, placeholder references didn't get loaded - There's one super small comment that looks like it just got buried.
Edited by Sam Word - I had to rebase these changes with the other bitbucket server changes on
added 2270 commits
-
54638be4...ff7b12a9 - 2265 commits from branch
master
- 1be8c4d8 - Add placeholder user mapping to bitbucket server importer
- f36256b7 - Add placeholder user mapping to bitbucket server [jnutt additions]
- 85e87983 - Spec updates for new default
- 40206ef7 - Add ID to user representations as unique-er ID
- 3bc1bdc1 - Revert back to using the slug
Toggle commit list-
54638be4...ff7b12a9 - 2265 commits from branch
requested review from @SamWord
mentioned in issue #506834
mentioned in issue #506870
requested review from @rodrigo.tomonari
added pipeline:mr-approved label
added pipelinetier-3 pipeline:run-e2e-omnibus-once labels and removed pipelinetier-1 label
Before you set this MR to auto-merge
This merge request will progress on pipeline tiers until it reaches the last tier: pipelinetier-3. We will trigger a new pipeline for each transition to a higher tier.
Before you set this MR to auto-merge, please check the following:
- You are the last maintainer of this merge request
- The latest pipeline for this merge request is pipelinetier-3 (You can find which tier it is in the pipeline name)
- This pipeline is recent enough (created in the last 8 hours)
If all the criteria above apply, please set auto-merge for this merge request.
See pipeline tiers and merging a merge request for more details.
E2E Test Result Summary
allure-report-publisher
generated test report!e2e-test-on-cng:
test report for 6c87badaexpand test summary
+------------------------------------------------------------------+ | suites summary | +-------------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +-------------+--------+--------+---------+-------+-------+--------+ | Plan | 86 | 0 | 8 | 0 | 94 | ✅ | | Govern | 84 | 0 | 9 | 1 | 93 | ✅ | | Create | 140 | 0 | 20 | 0 | 160 | ✅ | | Verify | 49 | 0 | 16 | 0 | 65 | ✅ | | Package | 24 | 0 | 14 | 0 | 38 | ✅ | | Data Stores | 33 | 0 | 10 | 0 | 43 | ✅ | | Analytics | 2 | 0 | 0 | 1 | 2 | ✅ | | Configure | 0 | 0 | 3 | 0 | 3 | ➖ | | Monitor | 8 | 0 | 12 | 0 | 20 | ✅ | | Manage | 1 | 0 | 9 | 0 | 10 | ✅ | | Fulfillment | 2 | 0 | 7 | 1 | 9 | ✅ | | ModelOps | 0 | 0 | 1 | 0 | 1 | ➖ | | Release | 5 | 0 | 1 | 0 | 6 | ✅ | | Secure | 2 | 0 | 5 | 0 | 7 | ✅ | | Ai-powered | 0 | 0 | 2 | 0 | 2 | ➖ | | Growth | 0 | 0 | 2 | 0 | 2 | ➖ | +-------------+--------+--------+---------+-------+-------+--------+ | Total | 436 | 0 | 119 | 3 | 555 | ✅ | +-------------+--------+--------+---------+-------+-------+--------+
e2e-test-on-gdk:
test report for 6c87badaexpand test summary
+------------------------------------------------------------------+ | suites summary | +-------------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +-------------+--------+--------+---------+-------+-------+--------+ | Govern | 150 | 0 | 6 | 0 | 156 | ✅ | | Create | 258 | 0 | 44 | 0 | 302 | ✅ | | Package | 50 | 0 | 22 | 0 | 72 | ✅ | | Verify | 86 | 0 | 4 | 0 | 90 | ✅ | | Plan | 152 | 0 | 0 | 0 | 152 | ✅ | | Monitor | 16 | 0 | 0 | 0 | 16 | ✅ | | Data Stores | 66 | 0 | 2 | 0 | 68 | ✅ | | Analytics | 4 | 0 | 0 | 0 | 4 | ✅ | | Fulfillment | 4 | 0 | 0 | 0 | 4 | ✅ | | Manage | 2 | 0 | 2 | 0 | 4 | ✅ | | Release | 10 | 0 | 0 | 0 | 10 | ✅ | | Secure | 8 | 0 | 0 | 0 | 8 | ✅ | | Ai-powered | 0 | 0 | 2 | 0 | 2 | ➖ | +-------------+--------+--------+---------+-------+-------+--------+ | Total | 806 | 0 | 82 | 0 | 888 | ✅ | +-------------+--------+--------+---------+-------+-------+--------+
- Resolved by James Nutt
reset approvals from @SamWord by pushing to the branch
mentioned in merge request !174462 (merged)
Generated bygitlab_quality-test_tooling
.
Slow tests detected in this merge request. These slow tests might be related to this merge request's changes.Click to expand
Job File Name Duration Expected duration #8534353141 spec/features/discussion_comments/merge_request_spec.rb#L20
Thread Comments Merge Request behaves like thread comments for issue, epic and merge request clicking "Comment" will post a comment 50.69 s < 50.13 s - A deleted user
added rspec:slow test detected label
started a merge train
mentioned in commit c7809c39
mentioned in issue #477554 (closed)
added workflowstaging-canary 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-staging label and removed workflowproduction label
added workflowpost-deploy-db-production label and removed workflowpost-deploy-db-staging label
added releasedcandidate label
added releasedpublished label and removed releasedcandidate label
15 15 event_id: merge_event[:id] 16 16 ) 17 17 18 committer = merge_event[:committer_email] 18 user_id = if user_mapping_enabled?(project) 19 user_finder.uid( 20 username: merge_event[:committer_username], @jnutt We have some Sentry errors https://new-sentry.gitlab.net/organizations/gitlab/issues/1254442 where we're attempting to create a source user with a
nil
source user identifier. This would be traced back tomerge_event[:committer_username]
beingnil
here - have we QA'd that a merge event is successfully created with user mapping? Or can the value returned here by the BBS API benull
sometimes - and if so would we map to a ghost user (after checking with Magda)?@.luke I'm fairly certain we QA'd with merge events. There is a service user that can be returned by the API, but in this case I would expect the username to still be present.
Allow me to investigate.
Updates:
-
committer_username
comes fromcommit.committer.slug
, which I've confirmed is present on my MRs for merge events. - I thought it might be related to merge strategy. Using fast-forward == no merge commit == no committer username? MRs created using FF or rebase & FF both seem to contain the correct commit info, though.
- I tried merging an MR and then deleting the user, and this does change how the committer data is returned. In the cases we've observed so far, the deleted user's original data is preserved in API responses, but I guess not here.
{ "name": "jnutt", "emailAddress": "jnutt@gitlab.com", "active": true, "displayName": "James Nutt", "id": 3, "slug": "jnutt", "type": "NORMAL", "links": { "self": [ { "href": "http://localhost:7990/users/jnutt" } ] } }
vs.
{ "name": "deleteme2", "emailAddress": "deleteme2@example.com" }
In that case, I suppose we'd map to a ghost user (semi-related issue) or similar. @m_frankiewicz, could you remind me what we do with contributions by deleted users in other importers?
Edited by James Nutt-
@jnutt We have an epic &16106. It might be worth us forming some very clear guidelines around how we should handle
null
users, and writing it into that epic description and our import design developer doc.It might something like:
- If we get a
null
user:- and a
null
user is possible (for example anull
assignee) do not map. - and a
null
user should not be possible (for example, an actor in an event) we interpret this as a deleted user and map to GitLab ghost user.
- and a
- If the source has a concept of a ghost user and returns this user in its response (like GitHub does sometimes for its
ghost
user) then map to GitLab ghost user.
For #506654 (closed) which is an issue in that epic, I was trying to modify the GitHub
UserFinder
to do the above #506654 (comment 2260295014), for a low-level-ish method to take aghost:
param to decide whether it returns the GitLab ghost user ornil
, and keep some the convenience wrapper for returning authors so it always returns ghost users whennull
.@m_frankiewicz How does the above sound? We can formalize what we decide in the epic description and our importer design principles doc, and can review what needs to be done across our importers to be consistent.
- If we get a
We can handle this error while we work out how best to handle
nil
users (I think these errors would signal data loss so it's worth fixing quickly): Handle missing users when user mapping in Bitbu... (!177321 - merged). It will map to the project creator which is the existing behaviour.Edited by Luke DuncalfeHow does the above sound? We can formalize what we decide in the epic description and our importer design principles doc, and can review what needs to be done across our importers to be consistent.
Sounds very good to me!
I don't understand one case from those you gave:
If we get a
null
user:- and a
null
user is possible (for example anull
assignee) do not map.
Does that mean that on source there was an assignee, but the source gives
null
for this user, so better not do add any assignee on the destination, rather than ghost user as assignee?Other examples - understood and agree.
- and a
mentioned in commit 986e0027
mentioned in merge request !177321 (merged)
mentioned in commit 28ac706e
mentioned in commit ae7955e6