-
Notifications
You must be signed in to change notification settings - Fork 97
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
Conversation
target-api-url
commands
Unit Test Results805 tests 805 ✅ 22s ⏱️ Results for commit e2befc6. ♻️ This comment has been updated with latest results. |
target-api-url
commandstarget-api-url
commands (-1)
There was a problem hiding this 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!
There was a problem hiding this 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.
src/Octoshift/Commands/AbortMigration/AbortMigrationCommandBase.cs
Outdated
Show resolved
Hide resolved
src/Octoshift/Commands/WaitForMigration/WaitForMigrationCommandBase.cs
Outdated
Show resolved
Hide resolved
src/Octoshift/Commands/RevokeMigratorRole/RevokeMigratorRoleCommandBase.cs
Outdated
Show resolved
Hide resolved
…e.cs Co-authored-by: Arin Ghazarian <[email protected]>
…dBase.cs Co-authored-by: Arin Ghazarian <[email protected]>
…mmandBase.cs Co-authored-by: Arin Ghazarian <[email protected]>
okay I have one more PR so will update to virtual for any others missed there 👍🏾 |
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.
ThirdPartyNotices.txt
(if applicable)