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

Support remaining target-api-url commands (-1) #1219

Merged
merged 9 commits into from
Feb 7, 2024
Merged

Conversation

brianaj
Copy link
Collaborator

@brianaj brianaj commented Feb 6, 2024

Last part of #1094 and https://github.ghe.com/github/octoshift/issues/7956

Shipping remaining commands in https://github.ghe.com/github/octoshift/issues/7956#issuecomment-8111321 except generate-script as that is more involved and will ship changes in separate PR.

Decided not to functional test since that is more involved and timely and we have shipped enough similar commands that unit testing should be good enough.

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

@brianaj brianaj changed the title RevokeMigratorRoleCommand changes Support remaining target-api-url commands Feb 6, 2024
Copy link

github-actions bot commented Feb 6, 2024

Unit Test Results

805 tests   805 ✅  22s ⏱️
  1 suites    0 💤
  1 files      0 ❌

Results for commit e2befc6.

♻️ This comment has been updated with latest results.

@brianaj brianaj marked this pull request as ready for review February 6, 2024 23:10
@brianaj brianaj changed the title Support remaining target-api-url commands Support remaining target-api-url commands (-1) Feb 6, 2024
Copy link

@gargola gargola left a comment

Choose a reason for hiding this comment

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

I'm not familiar with this code but after scanning the changes, nothing obviously concerning popped up, plus; as you said, this is something that has been done several times in the past. LGTM!

Copy link
Collaborator

@ArinGhazarian ArinGhazarian left a comment

Choose a reason for hiding this comment

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

Looks good, the only thing is the public virtual comment. We might have missed it in the previous PRs too.

@brianaj
Copy link
Collaborator Author

brianaj commented Feb 7, 2024

Looks good, the only thing is the public virtual comment. We might have missed it in the previous PRs too.

okay I have one more PR so will update to virtual for any others missed there 👍🏾

@brianaj brianaj enabled auto-merge February 7, 2024 01:21
@brianaj brianaj merged commit feba349 into main Feb 7, 2024
27 of 28 checks passed
@brianaj brianaj deleted the 1094-remaining-commands branch February 7, 2024 01:41
Copy link

github-actions bot commented Feb 7, 2024

Code Coverage

Package Line Rate Branch Rate Complexity Health
Octoshift 87% 76% 1270
gei 79% 70% 513
bbs2gh 78% 73% 647
ado2gh 84% 79% 616
Summary 83% (6788 / 8147) 75% (1520 / 2026) 3046

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

Successfully merging this pull request may close these issues.

3 participants