-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Add option to delete local and remote tag #4217
Add option to delete local and remote tag #4217
Conversation
c2d3d84
to
dfe7b45
Compare
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.
Looking good, see comments
Close() | ||
}). | ||
|
||
// Delete both local and remote tag new-tag-2 |
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.
Two things:
- I would create this as a separate test because it's fairly independent from what's already in this test.
- We need an assertion that the tag has been removed from the remote. Funnily enough I think that we don't actually show remote tags anywhere, so we'll need to instead settle for a shell command (you can add a function in pkg/integration/components/shell.go called AssertRemoteTagNotFound)
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.
Thank you for the review!
Sure, I agree with you. I will create a separate test and add a suggested assertion
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.
Done, see be5b9ad
@@ -87,6 +87,63 @@ var CrudLightweight = NewIntegrationTest(NewIntegrationTestArgs{ | |||
Select(Contains("Delete local tag")). | |||
Confirm() | |||
}). | |||
IsEmpty(). | |||
|
|||
// Delete both local and remote tag new-tag-2 |
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 wouldn't bother testing this for the lightweight case as it makes no difference in our code.
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.
Removed, see be5b9ad
4f235c6
to
93fbc11
Compare
93fbc11
to
7db8fb8
Compare
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.
LGTM. Will merge after tests pass.
BTW I pushed a small change myself and squashed the commits into one
Thank you for the review! |
Option to delete both local and remote tag, closes #4203.
go generate ./...
)