Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Require the Bitbucket Server URL, project and repo name to always be passed for bbs2gh migrate-repo #1057

Merged
merged 2 commits into from
Jul 10, 2023

Conversation

timrogers
Copy link
Contributor

@timrogers timrogers commented Jul 8, 2023

This makes a breaking change to bbs2gh migrate-repo requiring the --bbs-server-url, --bbs-project and --bbs-repo to always be passed, even in the flows where the CLI doesn't talk to the Bitbucket Server API.

This change allows us to always construct the URL of the repository and then send it to the GitHub API when starting the migration.

That's important, because the URL allows us to intelligently pick the right repo to migrate from the archive, rather than picking the "first" repo in the fork hierarchy (which may be wrong!).

I did consider introducing a warning first and then making a breaking change, but I thought going straight to a breaking change was the right decision because:

  • This is currently in private beta, so I can easily contact affected users
  • We're getting ready to move to public beta very soon, and I want this to be "right" before the public beta
  • The impact of this change is limited, given that most users use the default end-to-end flow

Fixes #1056.

  • 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)

@@ -283,44 +283,6 @@ public void Errors_If_BbsServer_Url_Not_Provided_But_Smb_User_Is_Provided()
.WithMessage("*SSH*SMB*--bbs-server-url*");
}

[Fact]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We now validate in the Args class that all 3 arguments are always provided

@github-actions
Copy link

github-actions bot commented Jul 8, 2023

Unit Test Results

781 tests   781 ✔️  26s ⏱️
    1 suites      0 💤
    1 files        0

Results for commit 54cdc71.

♻️ This comment has been updated with latest results.

@timrogers timrogers marked this pull request as ready for review July 8, 2023 18:50
@timrogers
Copy link
Contributor Author

I've tested all three flows (end-to-end + upload-and-import + import-only) after this change, and they all behave as expected.

@timrogers
Copy link
Contributor Author

@dylan-smith
Copy link
Collaborator

Did you consider doing a 2-phase implementation (like we have done for other breaking changes). Where we first make those arguments optional when using --archive-path or --archive-url and spit out a warning if users don't provide the optional arguments.

Then at some point in the future come back and make them required.

@timrogers
Copy link
Contributor Author

timrogers commented Jul 8, 2023

Did you consider doing a 2-phase implementation (like we have done for other breaking changes). Where we first make those arguments optional when using --archive-path or --archive-url and spit out a warning if users don't provide the optional arguments.

Then at some point in the future come back and make them required.

I did consider it - but I feel like a more decisive approach is fair game during a limited private beta that is heading to public beta in the near future.

It's easy for me to contact affected users, so I thought a more immediate change made sense to fix what is a pretty nasty bug.

But I will think more on this over the weekend! Thanks for raising it.

@timrogers timrogers force-pushed the timrogers/require-bbs-server-repo-and-project branch 3 times, most recently from b44455f to 70af1ce Compare July 9, 2023 19:07
…passed for `bbs2gh migrate-repo`

This makes a breaking change to `bbs2gh migrate-repo` requiring
the `--bbs-server-url`, `--bbs-project` and `--bbs-repo` to always
be passed, even in the flows where the CLI doesn't talk to the
Bitbucket Server API.

This change is important because it allows us to always sent the
URL of the repository to the GitHub API when starting the
migration, which allows us to intelligently pick the right repo
to migrate from the archive.

Fixes #1056.
@timrogers timrogers force-pushed the timrogers/require-bbs-server-repo-and-project branch 2 times, most recently from 345a9c9 to 45c7787 Compare July 10, 2023 11:00
@timrogers timrogers requested a review from a team July 10, 2023 13:40
Copy link
Member

@dpmex4527 dpmex4527 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@github-actions
Copy link

Code Coverage

Package Line Rate Branch Rate Complexity Health
Octoshift 86% 76% 1206
bbs2gh 79% 75% 644
ado2gh 84% 82% 617
gei 80% 75% 526
Summary 83% (6564 / 7879) 77% (1538 / 2004) 2993

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants