Skip to content
Snippets Groups Projects

Add placeholder user mapping to bitbucket server importer

Merged George Koltsov requested to merge georgekoltsov/bitbucket-server-placeholder-users into master
1 unresolved thread

What does this MR do and why?

image

image

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.
  • 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.
Edited by James Nutt

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
  • Luke Duncalfe
  • Luke Duncalfe
  • Luke Duncalfe mentioned in merge request !167169 (closed)

    mentioned in merge request !167169 (closed)

  • Luke Duncalfe
  • Luke Duncalfe
  • James Nutt
  • James Nutt added 1 commit

    added 1 commit

    • 7f692c0f - Add placeholder user mapping to bitbucket server [jnutt additions]

    Compare with previous version

  • James Nutt added 1 commit

    added 1 commit

    • 6dfbd84a - Add placeholder user mapping to bitbucket server [jnutt additions]

    Compare with previous version

  • mentioned in issue #467512 (closed)

  • James Nutt added 1 commit

    added 1 commit

    • b0bf7407 - Add placeholder user mapping to bitbucket server [jnutt additions]

    Compare with previous version

  • James Nutt added 4817 commits

    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]

    Compare with previous version

  • James Nutt added 1 commit

    added 1 commit

    • 3df5719e - Add placeholder user mapping to bitbucket server [jnutt additions]

    Compare with previous version

  • James Nutt added 1 commit

    added 1 commit

    • 7e348197 - Add placeholder user mapping to bitbucket server [jnutt additions]

    Compare with previous version

  • Rodrigo Tomonari mentioned in merge request !167078 (merged)

    mentioned in merge request !167078 (merged)

  • James Nutt added 1 commit

    added 1 commit

    • b6abb0a2 - Add placeholder user mapping to bitbucket server [jnutt additions]

    Compare with previous version

  • James Nutt added 1 commit

    added 1 commit

    • 9322c501 - Add placeholder user mapping to bitbucket server [jnutt additions]

    Compare with previous version

  • changed milestone to %17.6

  • Carla Drago mentioned in merge request !170084 (merged)

    mentioned in merge request !170084 (merged)

  • James Nutt added 4483 commits

    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]

    Compare with previous version

  • James Nutt added 1 commit

    added 1 commit

    • fd8c98c8 - Add placeholder user mapping to bitbucket server [jnutt additions]

    Compare with previous version

  • James Nutt added 1 commit

    added 1 commit

    • cfb159f2 - Add placeholder user mapping to bitbucket server [jnutt additions]

    Compare with previous version

  • James Nutt added 1 commit

    added 1 commit

    • 8736e618 - Add placeholder user mapping to bitbucket server [jnutt additions]

    Compare with previous version

  • James Nutt added 1 commit

    added 1 commit

    • c85cfb80 - Add placeholder user mapping to bitbucket server [jnutt additions]

    Compare with previous version

  • James Nutt added 1 commit

    added 1 commit

    • f0e9d1ea - Add placeholder user mapping to bitbucket server [jnutt additions]

    Compare with previous version

  • James Nutt added 1 commit

    added 1 commit

    • 240c3d74 - Add placeholder user mapping to bitbucket server [jnutt additions]

    Compare with previous version

  • James Nutt
  • James Nutt added 1 commit

    added 1 commit

    • 9ff3c1c5 - Add placeholder user mapping to bitbucket server [jnutt additions]

    Compare with previous version

  • James Nutt added 1 commit

    added 1 commit

    • eca8c6a2 - Add placeholder user mapping to bitbucket server [jnutt additions]

    Compare with previous version

  • James Nutt requested review from @SamWord

    requested review from @SamWord

  • James Nutt added 1 commit

    added 1 commit

    • 0f326fbf - Add placeholder user mapping to bitbucket server [jnutt additions]

    Compare with previous version

  • James Nutt added 1437 commits

    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]

    Compare with previous version

  • James Nutt added 1 commit

    added 1 commit

    • 5aee825d - Add placeholder user mapping to bitbucket server [jnutt additions]

    Compare with previous version

  • James Nutt added 1 commit

    added 1 commit

    • 6253ef26 - Add placeholder user mapping to bitbucket server [jnutt additions]

    Compare with previous version

  • James Nutt added 1 commit

    added 1 commit

    • 859c30c3 - Add placeholder user mapping to bitbucket server [jnutt additions]

    Compare with previous version

  • James Nutt added 1 commit

    added 1 commit

    • 965dff79 - Add placeholder user mapping to bitbucket server [jnutt additions]

    Compare with previous version

  • James Nutt mentioned in merge request !171842 (merged)

    mentioned in merge request !171842 (merged)

  • James Nutt
  • James Nutt
  • James Nutt added 1 commit

    added 1 commit

    • 1ae767d4 - Add placeholder user mapping to bitbucket server [jnutt additions]

    Compare with previous version

  • James Nutt added 406 commits

    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]

    Compare with previous version

  • James Nutt added 1 commit

    added 1 commit

    • d74131ef - Add placeholder user mapping to bitbucket server [jnutt additions]

    Compare with previous version

  • Ghost User
  • James Nutt added 1 commit

    added 1 commit

    • 65e372fe - Add placeholder user mapping to bitbucket server [jnutt additions]

    Compare with previous version

  • James Nutt added 1 commit

    added 1 commit

    • 701712e2 - Add placeholder user mapping to bitbucket server [jnutt additions]

    Compare with previous version

  • James Nutt added 325 commits

    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]

    Compare with previous version

  • Sam Word
  • Sam Word
  • Sam Word
  • Sam Word
  • Sam Word
  • Sam Word
  • Sam Word
  • Sam Word
  • Sam Word
  • Sam Word
  • Sam Word requested changes

    requested changes

  • Sam Word removed review request for @SamWord

    removed review request for @SamWord

  • James Nutt changed milestone to %17.7

    changed milestone to %17.7

  • James Nutt added 1402 commits

    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]

    Compare with previous version

  • James Nutt added 1 commit

    added 1 commit

    • f28a380e - Add placeholder user mapping to bitbucket server [jnutt additions]

    Compare with previous version

  • James Nutt added 1 commit

    added 1 commit

    • 7b8a3da0 - Add placeholder user mapping to bitbucket server [jnutt additions]

    Compare with previous version

  • James Nutt added 1 commit

    added 1 commit

    • 48fefc21 - Add placeholder user mapping to bitbucket server [jnutt additions]

    Compare with previous version

  • James Nutt added 1 commit

    added 1 commit

    • c7713782 - Add placeholder user mapping to bitbucket server [jnutt additions]

    Compare with previous version

  • James Nutt added 1 commit

    added 1 commit

    • 53064493 - Add placeholder user mapping to bitbucket server [jnutt additions]

    Compare with previous version

  • James Nutt added 1 commit

    added 1 commit

    • bdcb3caa - Add placeholder user mapping to bitbucket server [jnutt additions]

    Compare with previous version

  • James Nutt added 1 commit

    added 1 commit

    • 6f825e87 - Add placeholder user mapping to bitbucket server [jnutt additions]

    Compare with previous version

  • James Nutt requested review from @SamWord

    requested review from @SamWord

  • James Nutt marked this merge request as ready

    marked this merge request as ready

  • James Nutt added 1 commit

    added 1 commit

    • d0284f13 - Add placeholder user mapping to bitbucket server [jnutt additions]

    Compare with previous version

  • James Nutt added 1 commit

    added 1 commit

    • c55e5b16 - Add placeholder user mapping to bitbucket server [jnutt additions]

    Compare with previous version

  • James Nutt added 1 commit

    added 1 commit

    • 3f109fb5 - Add placeholder user mapping to bitbucket server [jnutt additions]

    Compare with previous version

  • James Nutt added 1 commit

    added 1 commit

    • d491c757 - Add placeholder user mapping to bitbucket server [jnutt additions]

    Compare with previous version

  • James Nutt added 1 commit

    added 1 commit

    • 4ecdf221 - Add placeholder user mapping to bitbucket server [jnutt additions]

    Compare with previous version

  • Sam Word
  • Sam Word
  • Sam Word
  • Sam Word
  • Sam Word
  • Sam Word
  • Sam Word
  • Sam Word
  • James Nutt changed the description

    changed the description

  • James Nutt added 1 commit

    added 1 commit

    • 2bcf2ff9 - Add placeholder user mapping to bitbucket server [jnutt additions]

    Compare with previous version

  • James Nutt added 1 commit

    added 1 commit

    • d21e5076 - Add placeholder user mapping to bitbucket server [jnutt additions]

    Compare with previous version

  • Ghost User
  • James Nutt added 1 commit

    added 1 commit

    • 116b0cea - Add placeholder user mapping to bitbucket server [jnutt additions]

    Compare with previous version

  • James Nutt added 1 commit

    added 1 commit

    • 167b3b06 - Add placeholder user mapping to bitbucket server [jnutt additions]

    Compare with previous version

  • Sam Word
  • Sam Word
  • Sam Word
  • James Nutt added 2 commits

    added 2 commits

    • 50b9ecf0 - Add placeholder user mapping to bitbucket server [jnutt additions]
    • 86dbb98b - Spec updates for new default

    Compare with previous version

  • James Nutt added 1 commit

    added 1 commit

    • 63397286 - Spec updates for new default

    Compare with previous version

  • James Nutt added 1 commit

    added 1 commit

    • 5161b0ff - Spec updates for new default

    Compare with previous version

  • Ghost User
  • James Nutt added 1 commit

    added 1 commit

    • 6d24c49c - Spec updates for new default

    Compare with previous version

  • James Nutt requested review from @SamWord

    requested review from @SamWord

  • Sam Word
  • Sam Word
  • Sam Word
  • Sam Word
  • Sam Word
  • Sam Word
  • Sam Word
  • Sam Word
  • Sam Word
  • Sam Word
  • Sam Word
  • Sam Word
  • Sam Word
  • Sam Word
  • Sam Word
  • Sam Word
  • Sam Word
  • Sam Word
  • Sam Word
  • Sam Word
  • Sam Word
  • Sam Word
  • Sam Word
  • Sam Word
  • Sam Word
  • James Nutt added 1 commit

    added 1 commit

    • 81675348 - Add ID to user representations as unique-er ID

    Compare with previous version

  • James Nutt added 1 commit

    added 1 commit

    • f33ed1fb - Add ID to user representations as unique-er ID

    Compare with previous version

  • mentioned in issue #506719 (closed)

  • James Nutt added 1 commit

    added 1 commit

    • 200a8e97 - Add ID to user representations as unique-er ID

    Compare with previous version

  • James Nutt added 1 commit

    added 1 commit

    • 6127bd32 - Revert back to using the slug

    Compare with previous version

  • James Nutt added 1 commit

    added 1 commit

    • ac39e10d - Revert back to using the slug

    Compare with previous version

  • James Nutt added 1 commit

    added 1 commit

    • 54638be4 - Revert back to using the slug

    Compare with previous version

    • 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

      image

      image

      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
  • James Nutt added 2270 commits

    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

    Compare with previous version

  • James Nutt requested review from @SamWord

    requested review from @SamWord

  • James Nutt mentioned in issue #506834

    mentioned in issue #506834

  • James Nutt mentioned in issue #506870

    mentioned in issue #506870

  • Sam Word approved this merge request

    approved this merge request

  • Sam Word requested review from @rodrigo.tomonari

    requested review from @rodrigo.tomonari

  • 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: :white_check_mark: test report for 6c87bada

    expand 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: :white_check_mark: test report for 6c87bada

    expand 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   | ✅     |
    +-------------+--------+--------+---------+-------+-------+--------+
  • Sam Word
  • James Nutt added 1 commit

    added 1 commit

    Compare with previous version

  • James Nutt reset approvals from @SamWord by pushing to the branch

    reset approvals from @SamWord by pushing to the branch

  • Looks like there are a couple undercoverage issues in the specs for the old behaviour. :frowning2: I'll fix them now.

  • James Nutt added 1 commit

    added 1 commit

    Compare with previous version

  • Rodrigo Tomonari approved this merge request

    approved this merge request

  • Sam Word mentioned in merge request !174462 (merged)

    mentioned in merge request !174462 (merged)

  • Rodrigo Tomonari resolved all threads

    resolved all threads

  • Rodrigo Tomonari enabled automatic add to merge train when checks pass

    enabled automatic add to merge train when checks pass

  • :tools: Generated by gitlab_quality-test_tooling.


    :snail: 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
  • mentioned in commit c7809c39

  • mentioned in issue #477554 (closed)

  • added workflowstaging label and removed workflowcanary 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 to merge_event[:committer_username] being nil 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 be null 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. :thinking:

      Allow me to investigate.

      Updates:

      • committer_username comes from commit.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 a null 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.
      • 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 a ghost: param to decide whether it returns the GitLab ghost user or nil, and keep some the convenience wrapper for returning authors so it always returns ghost users when null.

      @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.

    • 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 Duncalfe
    • @.luke @jnutt

      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.

      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 a null 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. :pray_tone1:

    • Please register or sign in to reply
  • Luke Duncalfe mentioned in commit 986e0027

    mentioned in commit 986e0027

  • Luke Duncalfe mentioned in merge request !177321 (merged)

    mentioned in merge request !177321 (merged)

  • Luke Duncalfe mentioned in commit 28ac706e

    mentioned in commit 28ac706e

  • Luke Duncalfe mentioned in commit ae7955e6

    mentioned in commit ae7955e6

  • Please register or sign in to reply
    Loading