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

Add option to delete local and remote tag #4217

Merged

Conversation

AnvarU
Copy link
Contributor

@AnvarU AnvarU commented Jan 27, 2025

  • PR Description

Option to delete both local and remote tag, closes #4203.

  • Please check if the PR fulfills these requirements
  • Cheatsheets are up-to-date (run go generate ./...)
  • Code has been formatted (see here)
  • Tests have been added/updated (see here for the integration test guide)
  • Text is internationalised (see here)
  • If a new UserConfig entry was added, make sure it can be hot-reloaded (see here)
  • Docs have been updated if necessary
  • You've read through your own file changes for silly mistakes etc

@AnvarU AnvarU force-pushed the feature/delete-local-remote-tag branch from c2d3d84 to dfe7b45 Compare January 27, 2025 16:48
@AnvarU
Copy link
Contributor Author

AnvarU commented Jan 27, 2025

Some screens with workflow:

  1. Select Delete local and remote tag Снимок экрана 2025-01-27 в 22 58 28
  2. Choose a remote from which to remove a tag Снимок экрана 2025-01-27 в 22 58 46
  3. Confirm deletion of the tag from local machine and from remote Снимок экрана 2025-01-27 в 22 59 00

Copy link
Owner

@jesseduffield jesseduffield left a 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

pkg/i18n/english.go Outdated Show resolved Hide resolved
Close()
}).

// Delete both local and remote tag new-tag-2
Copy link
Owner

Choose a reason for hiding this comment

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

Two things:

  1. I would create this as a separate test because it's fairly independent from what's already in this test.
  2. 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)

Copy link
Contributor Author

@AnvarU AnvarU Jan 29, 2025

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

Copy link
Contributor Author

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
Copy link
Owner

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed, see be5b9ad

@jesseduffield jesseduffield force-pushed the feature/delete-local-remote-tag branch from 4f235c6 to 93fbc11 Compare January 29, 2025 22:04
@jesseduffield jesseduffield force-pushed the feature/delete-local-remote-tag branch from 93fbc11 to 7db8fb8 Compare January 29, 2025 22:05
@jesseduffield jesseduffield added the enhancement New feature or request label Jan 29, 2025
Copy link
Owner

@jesseduffield jesseduffield left a 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

@AnvarU
Copy link
Contributor Author

AnvarU commented Jan 29, 2025

Thank you for the review!

@jesseduffield jesseduffield merged commit d2723ff into jesseduffield:master Jan 29, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

An option to delete both local and remote tag at once
2 participants