Properly migrate forks by passing BBS repo URL to GEI by dpmex4527 · Pull Request #1002 · github/gh-gei

dpmex4527 · 2023-05-23T21:06:57Z

This PR updates the value passed in the sourceRepositoryUrl field when starting a repository migration in GEI to include the full BBS repo URL and is constructed using the values from the --bbs-server-url, --bbs-project, and --bbs-repo cli args. This change is being made, alongside upcoming GEI changes in the backend, to allow customers to properly migrate repositories that either are currently forks or used to be a fork but the parent repo was deleted.

The Bitbucket Server export API documentation states that when a fork is selected for export, all repos that are part of the fork network will be included in the export archive, which includes the parent repo and any other child forks of the parent repo. This resulted in a few error scenarios such as when a migration would fail with the Multiple repositories found in archive error or if the migration succeeded but instead of migrating the fork git data, the parent git data was picked instead.

This is my first contribution to the CLI so looking forward to any feedback!

Did you write/update appropriate tests
Release notes updated (if appropriate)
Appropriate logging output
Issue linked
Docs updated (or issue created)
New package licenses are added to ThirdPartyNotices.txt (if applicable)

- Upcoming backend changes will allow customers to select which repo to migrate if migrating a fork based on the --bbs-project and --bbs-repo flags passed in. The cli will craft the full repo url that contains the necessary context so that the backend correctly choose which repo to migrate in an export archive as Bitbucket Server always includes the full fork network in export archives that are generated. - This solves the "Multiple Repsitories in archive" error and also solves the problem where a customer wants to migrate a source but octoshift incorrectly picks the parent repo git data. - Update tests

github-actions · 2023-05-23T21:28:06Z

Unit Test Results

780 tests 780 ✔️ 21s ⏱️
    1 suites     0 💤
    1 files     0 ❌

Results for commit 46eafd7.

♻️ This comment has been updated with latest results.

- Use BBS_SERVER_URL instead of BBS_HOST when setting up BBS source URL

timrogers

LGTM (but do get a review from an engineer first 😊 !)

dylan-smith · 2023-05-24T05:50:14Z

We support calling bbs2gh in a few different "modes" depending on the args you pass (we have unit tests for them all - hence the test failures). One of those is "ingest-only" where we give it the url or path of an archive but no project or repo name. You'll have to figure out what we want to do there. If you make project/repo mandatory you will break compat for existing users.

timrogers · 2023-05-24T06:11:58Z

We support calling bbs2gh in a few different "modes" depending on the args you pass (we have unit tests for them all - hence the test failures). One of those is "ingest-only" where we give it the url or path of an archive but no project or repo name. You'll have to figure out what we want to do there. If you make project/repo mandatory you will break compat for existing users.

That's a great point! For what it's worth, I'm happy to introduce a breaking change at this point in the private beta, as it will enable us to make migrations more reliable and avoid surprises.

…as arguments - Update tests to address NullReference exceptions

dpmex4527 · 2023-05-25T15:53:28Z

👋 @timrogers @dylan-smith thanks for the feedback! I'm 👍 in making the --bbs-project, --bbs-repo, and --bbs-server-url required parameters (and thus introduce breaking changes) but in hopes of getting this change out (and backend changes out later today), I can do this as a followup PR. I've gone ahead and fixed the System.NullReferenceException errors in CI by ensuring that all three args are passed in when generating repo URL and falling back to old https://not-used URL if any one of these args are not passed in.

dpmex4527 · 2023-05-25T16:28:28Z

I've opened #1004 to track making the --bbs-server-url, --bbs-project, and --bbs-repo required arguments.

synthead

This looks awesome! Quick and straight-forward 😄

I left a suggestion to remove the "not used" values for the fields that are now optional! Feel free to make these changes, or file a tech debit issue for them and ship this PR as-is. I left a LGTM on this review so you can make a choice 👍

synthead · 2023-05-25T16:34:45Z

src/Octoshift/Services/GithubApi.cs

+            bbsRepoUrl,  // source repository URL
            orgId,
            repo,
            "not-used",  // source access token


The access token and metadata URL fields are optional in the GraphQL API now! I know that these fields are mostly unrelated to your changes, but it would be great to switch them to null while you're here! If you'd like, you are also welcome to file a tech debt issue for this suggestion instead and ship this PR without this suggestion 👍

The biggest lift would be to remove the ! in the query in the GithubApi to for accessToken indicate that they're optional:

gh-gei/src/Octoshift/Services/GithubApi.cs

Line 294 in cff1488

$accessToken: String!,

There are various places in tests where we build a query, and we'll also want to remove the !. This should be a simple find-and-replace. Here's an example:

gh-gei/src/OctoshiftCLI.Tests/Octoshift/Services/GithubApiTests.cs

Line 889 in cff1488

$accessToken: String!,

Then, for the BBS tests, we'd want to remove the "not used" variables:

gh-gei/src/OctoshiftCLI.Tests/Octoshift/Services/GithubApiTests.cs

Lines 877 to 878 in b709e7b

const string unusedSourceToken = "not-used";

const string unusedMetadataArchiveUrl = "https://not-used";

For the metadataArchiveUrl and accessToken values here...

gh-gei/src/OctoshiftCLI.Tests/Octoshift/Services/GithubApiTests.cs

Lines 934 to 935 in b709e7b

metadataArchiveUrl = unusedMetadataArchiveUrl,

accessToken = unusedSourceToken,

...we'll simply turn this into a hard-coded, nulled string. There's a good example of that here:

gh-gei/src/OctoshiftCLI.Tests/Octoshift/Services/GithubApiTests.cs

Line 938 in b709e7b

targetRepoVisibility = (string)null,

I made these changes (with below diff) but now getting null reference exceptions. Any idea on how to best resolve that?

@dpmex4527 ➜ /workspaces/gh-gei (bbs-send-repo-url-for-selecting-fork-support) $ git diff diff --git a/src/Octoshift/Services/GithubApi.cs b/src/Octoshift/Services/GithubApi.cs index 323d543..ea0fe65 100644 --- a/src/Octoshift/Services/GithubApi.cs +++ b/src/Octoshift/Services/GithubApi.cs @@ -291,7 +291,7 @@ public class GithubApi $continueOnError: Boolean!, $gitArchiveUrl: String, $metadataArchiveUrl: String, - $accessToken: String!, + $accessToken: String, $githubPat: String, $skipReleases: Boolean, $targetRepoVisibility: String, diff --git a/src/OctoshiftCLI.Tests/Octoshift/Services/GithubApiTests.cs b/src/OctoshiftCLI.Tests/Octoshift/Services/GithubApiTests.cs index 76665b1..e447bc2 100644 --- a/src/OctoshiftCLI.Tests/Octoshift/Services/GithubApiTests.cs +++ b/src/OctoshiftCLI.Tests/Octoshift/Services/GithubApiTests.cs @@ -778,7 +778,7 @@ public class GithubApiTests $continueOnError: Boolean!, $gitArchiveUrl: String, $metadataArchiveUrl: String, - $accessToken: String!, + $accessToken: String, $githubPat: String, $skipReleases: Boolean, $targetRepoVisibility: String, @@ -874,9 +874,6 @@ public class GithubApiTests const string gitArchiveUrl = "GIT_ARCHIVE_URL"; const string targetToken = "TARGET_TOKEN"; - const string unusedSourceToken = "not-used"; - const string unusedMetadataArchiveUrl = "https://not-used"; - const string query = @" mutation startRepositoryMigration( $sourceId: ID!, @@ -886,7 +883,7 @@ public class GithubApiTests $continueOnError: Boolean!, $gitArchiveUrl: String, $metadataArchiveUrl: String, - $accessToken: String!, + $accessToken: String, $githubPat: String, $skipReleases: Boolean, $targetRepoVisibility: String, @@ -931,8 +928,8 @@ public class GithubApiTests repositoryName = GITHUB_REPO, continueOnError = true, gitArchiveUrl, - metadataArchiveUrl = unusedMetadataArchiveUrl, - accessToken = unusedSourceToken, + metadataArchiveUrl = (string)null, + accessToken = (string)null, githubPat = targetToken, skipReleases = false, targetRepoVisibility = (string)null,

@dpmex4527 ➜ /workspaces/gh-gei (bbs-send-repo-url-for-selecting-fork-support) $ dotnet test src/OctoshiftCLI.Tests/OctoshiftCLI.Tests.csproj --no-build --verbosity normal Build started 5/25/2023 8:20:16 PM. Test run for /workspaces/gh-gei/src/OctoshiftCLI.Tests/bin/Debug/net6.0/OctoshiftCLI.Tests.dll (.NETCoreApp,Version=v6.0) Microsoft (R) Test Execution Command Line Tool Version 17.3.2 (x64) Copyright (c) Microsoft Corporation. All rights reserved. Starting test execution, please wait... A total of 1 test files matched the specified pattern. Unexpected response when retrieving User ID {"invalid":{"PublicAlias":{"value":"foo-user-id"}}} Description: Adds a team to a repo with a specific role/permission Note: Expects GH_PAT env variable or --github-pat option to be set. Usage: add-team-to-repo [options] Options: --github-org <github-org> (REQUIRED) --github-repo <github-repo> (REQUIRED) --team <team> (REQUIRED) --role <admin|maintain|pull|push|triage> (REQUIRED) The only valid values are: pull, push, admin, maintain, triage. For more details see https://docs.github.com/en/rest/reference/teams#add-or-update-team-repository-permissions, custom repository roles are not currently supported. --github-pat <github-pat> --verbose --version Show version information -?, -h, --help Show help and usage information [xUnit.net 00:00:01.46] OctoshiftCLI.Tests.Octoshift.Services.GithubApiTests.StartBbsMigration_Returns_New_Repository_Migration_Id [FAIL] Failed OctoshiftCLI.Tests.Octoshift.Services.GithubApiTests.StartBbsMigration_Returns_New_Repository_Migration_Id [11 ms] Error Message: System.NullReferenceException : Object reference not set to an instance of an object. Stack Trace: at OctoshiftCLI.Services.GithubApi.StartMigration(String migrationSourceId, String sourceRepoUrl, String orgId, String repo, String sourceToken, String targetToken, String gitArchiveUrl, String metadataArchiveUrl, Boolean skipReleases, String targetRepoVisibility, Boolean lockSource) in /workspaces/gh-gei/src/Octoshift/Services/GithubApi.cs:line 352 at OctoshiftCLI.Services.GithubApi.StartBbsMigration(String migrationSourceId, String bbsRepoUrl, String orgId, String repo, String targetToken, String archiveUrl, String targetRepoVisibility) in /workspaces/gh-gei/src/Octoshift/Services/GithubApi.cs:line 428 at OctoshiftCLI.Tests.Octoshift.Services.GithubApiTests.StartBbsMigration_Returns_New_Repository_Migration_Id() in /workspaces/gh-gei/src/OctoshiftCLI.Tests/Octoshift/Services/GithubApiTests.cs:line 965 --- End of stack trace from previous location --- Failed! - Failed: 1, Passed: 777, Skipped: 0, Total: 778, Duration: 7 s - /workspaces/gh-gei/src/OctoshiftCLI.Tests/bin/Debug/net6.0/OctoshiftCLI.Tests.dll (net6.0) Build FAILED. 0 Warning(s) 0 Error(s) Time Elapsed 00:00:09.33

You know, I was working on this myself to help you resolve these test failures, and it turned out to be quite a bit more work than I expected 😄 Let's hold off on this for now and remove the "not used" parameters in a follow-up PR 👍

Sounds good. I've added this as a cleanup task in #1004

src/bbs2gh/Commands/MigrateRepo/MigrateRepoCommandHandler.cs

synthead

Sorry to make a "request changes" review after approving your PR 😅 I just saw that this PR doesn't have coverage for the backward-compatible BBS repo URL functionality that this PR introduces. Could we add a few tests for it?

src/bbs2gh/Commands/MigrateRepo/MigrateRepoCommandHandler.cs

Co-authored-by: Maxwell Pray <synthead@github.com>

src/OctoshiftCLI.Tests/bbs2gh/Commands/MigrateRepo/MigrateRepoCommandHandlerTests.cs

- Test passes without it

…CommandHandlerTests.cs Co-authored-by: Dylan Smith <dylanfromwinnipeg@gmail.com>

…//github.com/github/gh-gei into bbs-send-repo-url-for-selecting-fork-support

dylan-smith · 2023-05-25T20:58:20Z

release notes?

dpmex4527 · 2023-05-25T21:06:49Z

Release note?

Added in 8d5d503. Let me know if wording sounds good or if it needs updates.

synthead · 2023-05-25T19:59:24Z

src/bbs2gh/Commands/MigrateRepo/MigrateRepoCommandHandler.cs

+    private string GetBbsRepoUrl(MigrateRepoCommandArgs args)
+    {
+        return !args.BbsServerUrl.HasValue() || !args.BbsProject.HasValue() || !args.BbsRepo.HasValue()
+            ? "https://not-used"
+            : $"{args.BbsServerUrl.TrimEnd('/')}/projects/{args.BbsProject}/repos/{args.BbsRepo}/browse";
+    }


Since we're sticking with a ternary as opposed to a guard (thanks dotnet format 😄), I think this reads a little better without "not" operators and inverting the logic:

Suggested change

private string GetBbsRepoUrl(MigrateRepoCommandArgs args)

{

return !args.BbsServerUrl.HasValue() || !args.BbsProject.HasValue() || !args.BbsRepo.HasValue()

? "https://not-used"

: $"{args.BbsServerUrl.TrimEnd('/')}/projects/{args.BbsProject}/repos/{args.BbsRepo}/browse";

}

private string GetBbsRepoUrl(MigrateRepoCommandArgs args)

{

return args.BbsServerUrl.HasValue() && args.BbsProject.HasValue() && args.BbsRepo.HasValue()

? $"{args.BbsServerUrl.TrimEnd('/')}/projects/{args.BbsProject}/repos/{args.BbsRepo}/browse"

: "https://not-used"

}

FYI there's also .IsNullOrWhitespace() which is the opposite of .HasValue()

Fixed in f0636ac

src/bbs2gh/Commands/MigrateRepo/MigrateRepoCommandHandler.cs

synthead · 2023-05-26T09:49:13Z

src/Octoshift/Services/GithubApi.cs

+            bbsRepoUrl,  // source repository URL
            orgId,
            repo,
            "not-used",  // source access token


You know, I was working on this myself to help you resolve these test failures, and it turned out to be quite a bit more work than I expected 😄 Let's hold off on this for now and remove the "not used" parameters in a follow-up PR 👍

- Refactor ternary expression to make it read better

synthead

One small suggestion in the release notes, then we're good to 🚢!

RELEASENOTES.md

Co-authored-by: Maxwell Pray <synthead@github.com>

synthead

LGTM!

github-actions · 2023-05-30T19:07:44Z

Package	Line Rate	Branch Rate	Complexity	Health
Octoshift	85%	75%	1168	✔
bbs2gh	79%	75%	573	✔
ado2gh	84%	82%	617	✔
gei	83%	79%	652	✔
Summary	84% (6491 / 7765)	77% (1572 / 2042)	3010	✔

Fix tests

3f41973

- Use BBS_SERVER_URL instead of BBS_HOST when setting up BBS source URL

timrogers approved these changes May 24, 2023

View reviewed changes

dpmex4527 added 3 commits May 24, 2023 17:48

Merge branch 'main' into bbs-send-repo-url-for-selecting-fork-support

7476a30

Only pass BBS repo url if serverUrl, project, and repo are passed in …

9c2aa74

…as arguments - Update tests to address NullReference exceptions

Merge branch 'main' into bbs-send-repo-url-for-selecting-fork-support

b709e7b

dpmex4527 marked this pull request as ready for review May 25, 2023 16:20

dpmex4527 self-assigned this May 25, 2023

dpmex4527 mentioned this pull request May 25, 2023

Make --bbs-server-url, --bbs-project-, and --bbs-repo required arguments for bbs2gh migrate-repo command #1004

Closed

3 tasks

synthead approved these changes May 25, 2023

View reviewed changes

synthead reviewed May 25, 2023

View reviewed changes

src/bbs2gh/Commands/MigrateRepo/MigrateRepoCommandHandler.cs Outdated Show resolved Hide resolved

synthead requested changes May 25, 2023

View reviewed changes

src/bbs2gh/Commands/MigrateRepo/MigrateRepoCommandHandler.cs Show resolved Hide resolved

dpmex4527 and others added 2 commits May 25, 2023 12:26

Update src/bbs2gh/Commands/MigrateRepo/MigrateRepoCommandHandler.cs

687b1e4

Co-authored-by: Maxwell Pray <synthead@github.com>

Add missing semicolon

35e17e2

dylan-smith reviewed May 25, 2023

View reviewed changes

src/OctoshiftCLI.Tests/bbs2gh/Commands/MigrateRepo/MigrateRepoCommandHandlerTests.cs Outdated Show resolved Hide resolved

dylan-smith reviewed May 25, 2023

View reviewed changes

src/OctoshiftCLI.Tests/bbs2gh/Commands/MigrateRepo/MigrateRepoCommandHandlerTests.cs Outdated Show resolved Hide resolved

dpmex4527 and others added 4 commits May 25, 2023 17:48

Use suggested conditional expression from linter

68dd277

Remove setup call that is not needed

dd0ac88

- Test passes without it

Update src/OctoshiftCLI.Tests/bbs2gh/Commands/MigrateRepo/MigrateRepo…

bf3e0c3

…CommandHandlerTests.cs Co-authored-by: Dylan Smith <dylanfromwinnipeg@gmail.com>

Merge branch 'bbs-send-repo-url-for-selecting-fork-support' of https:…

475d754

…//github.com/github/gh-gei into bbs-send-repo-url-for-selecting-fork-support

dpmex4527 requested review from synthead and timrogers May 25, 2023 18:00

Add release note for fork support

8d5d503

synthead reviewed May 26, 2023

View reviewed changes

dpmex4527 added 2 commits May 26, 2023 14:48

Roll in feedback from @synthead

f0636ac

- Refactor ternary expression to make it read better

Merge branch 'main' into bbs-send-repo-url-for-selecting-fork-support

b7f919d

dpmex4527 requested review from dylan-smith and synthead May 26, 2023 14:52

Merge branch 'main' into bbs-send-repo-url-for-selecting-fork-support

9e56a9f

synthead reviewed May 30, 2023

View reviewed changes

RELEASENOTES.md Outdated Show resolved Hide resolved

Update RELEASENOTES.md

46eafd7

Co-authored-by: Maxwell Pray <synthead@github.com>

dpmex4527 requested a review from synthead May 30, 2023 18:14

synthead approved these changes May 30, 2023

View reviewed changes

dpmex4527 enabled auto-merge May 30, 2023 18:16

dpmex4527 merged commit 2d78ca3 into main May 30, 2023

dpmex4527 deleted the bbs-send-repo-url-for-selecting-fork-support branch May 30, 2023 19:06

	const string unusedSourceToken = "not-used";
	const string unusedMetadataArchiveUrl = "https://not-used";

	metadataArchiveUrl = unusedMetadataArchiveUrl,
	accessToken = unusedSourceToken,

Conversation

dpmex4527 commented May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Test Results

Uh oh!

timrogers left a comment

Choose a reason for hiding this comment

Uh oh!

dylan-smith commented May 24, 2023

Uh oh!

timrogers commented May 24, 2023

Uh oh!

dpmex4527 commented May 25, 2023

Uh oh!

dpmex4527 commented May 25, 2023

Uh oh!

synthead left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

synthead left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dylan-smith commented May 25, 2023

Uh oh!

dpmex4527 commented May 25, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

synthead left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

synthead left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented May 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dpmex4527 commented May 23, 2023 •

edited

Loading

github-actions bot commented May 23, 2023 •

edited

Loading