Skip to content

Push tag before release if found locally - #4571

Closed
guerinoni wants to merge 1 commit into
cli:trunkfrom
guerinoni:push-tag-locally-before-release
Closed

guerinoni wants to merge 1 commit into
cli:trunkfrom
guerinoni:push-tag-locally-before-release

Conversation

@guerinoni

Copy link
Copy Markdown
Contributor

Fixes: #4357
Signed-off-by: Federico Guerinoni [email protected]

@guerinoni
guerinoni requested a review from a team as a code owner October 19, 2021 22:32
@guerinoni
guerinoni requested review from vilmibm and removed request for a team October 19, 2021 22:32
@spenserblack

spenserblack commented Oct 20, 2021 •

Copy link
Copy Markdown
Contributor

Checked behavior as asked in #4357 (comment). Looks like this does push the tag if it exists locally. Remote tag has message and signature.
Thanks!

@guerinoni

Copy link
Copy Markdown
Contributor Author

@vilmibm sorry for ping Nate, do you have any updates about this? :)

@mislav mislav added the external pull request originating outside of the CLI core team label Dec 20, 2021
@samcoe samcoe assigned samcoe and unassigned vilmibm Jan 14, 2022
@samcoe
samcoe self-requested a review January 14, 2022 09:55

@samcoe samcoe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@guerinoni This is coming along nicely. I left a couple comments, nothing major though. Additionally, it would be great to see some tests around this functionality.

Comment thread pkg/cmd/release/create/create.go Outdated
Comment thread pkg/cmd/release/create/create.go Outdated
Comment thread pkg/cmd/release/create/create.go
Comment thread pkg/cmd/release/create/create.go Outdated

@mislav mislav left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Apologies for jumping on this thread so late down the line, but I'd like to propose an alternate solution for the original problem, which was:

When using gh release create with a tag that exists in the local repository but not in origin, it creates a new tag instead of pushing the local tag. This seemed like confusing behavior to me.

I agree that it can be confusing that release create would ignore the local tag and create a new one from the state of the main branch. However, I do not think that auto-pushing the local tag is a great solution either since that approach raises new problems:

  • GitHub CLI generally avoids doing surprising operations under the hood. gh release create doing an implicit git push, while potentially helpful, can definitely be surprising to some.
  • How would someone intentionally ignore the local git tag (which may have been created for testing) in favor of generating a new git tag in the remote repository?
  • How would someone skip auto-pushing that happens by default? We would have to potentially add a new flag to control that, which complicates the whole feature.

I propose a slightly different approach: to let the release create command fail if it references a tag that exists locally but not on the remote. An implementation of this could be: if a local tag exists and --target was not given, use the local tag name as the value for --target, which should fail if the tag wasn't pushed. Then, the user could opt out of this behavior by explicitly setting --target. How does that sound?

@spenserblack

spenserblack commented Jan 18, 2022 •

Copy link
Copy Markdown
Contributor

let the release create command fail if it references a tag that exists locally but not on the remote.

I didn't think about that in my initial issue, but that would resolve the issue, too 👍. Overall, the issue is not that gh specifically isn't pushing the tag, but that a new remote tag has been created implicitly. So, at least personally, I can say that an error/warning to remind me to push would sufficiently resolve the issue.

@guerinoni

guerinoni commented Jan 18, 2022 •

Copy link
Copy Markdown
Contributor Author

Thanks for feedback... If this is useful for gh I will implement today, but as I already notice community PR's are skipped for long time and this does not feel people involved.
Btw, let me know if I should close this or try an implementation

@mislav

mislav commented Jan 18, 2022

Copy link
Copy Markdown
Contributor

If this is useful for gh I will implement today

Please do; you can reuse this PR too (feel free to force-push to it if needed).

but as I already notice community PR's are skipped for long time and this does not feel people involved.

We're sorry for letting some PR sit open for a long time, but now we're committed to seeing this one through to completion!

@guerinoni
guerinoni requested a review from a team as a code owner January 18, 2022 23:29
@mislav
mislav removed the request for review from a team January 19, 2022 11:42
@guerinoni
guerinoni requested review from mislav and samcoe January 19, 2022 22:16
@samcoe

samcoe commented Jan 26, 2022

Copy link
Copy Markdown
Contributor

@guerinoni Thanks for the time and effort on this PR. Unfortunately, I think we are going to go a different direction with the implementation and more closely follow the path outlined here. I opened #5104 that moves in that direction.

@samcoe samcoe closed this Jan 26, 2022
@guerinoni
guerinoni deleted the push-tag-locally-before-release branch January 26, 2022 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

external pull request originating outside of the CLI core team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Should gh release create push the local tag if it exists?

6 participants