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

chore(cli): set version for root #1376

Merged
merged 2 commits into from
Nov 6, 2023
Merged

chore(cli): set version for root #1376

merged 2 commits into from
Nov 6, 2023

Conversation

cfabianski
Copy link
Collaborator

@cfabianski cfabianski commented Nov 6, 2023

Description

Setting the version will automatically enable bearer --version.
I found it particularly annoying/confusing in some occasions to not have it available.

image

https://github.com/spf13/cobra/blob/main/site/content/user_guide.md#version-flag

Cobra adds a top-level '--version' flag if the Version field is set on the root command. Running an application with the '--version' flag will print the version to stdout using the version template. The template can be customized using the cmd.SetVersionTemplate(s string) function.

Checklist

  • I've added test coverage that shows my fix or feature works as expected.
  • I've updated or added documentation if required.
  • I've included usage information in the description if CLI behavior was updated or added.
  • PR title follows Conventional Commits format

Copy link
Contributor

@gotbadger gotbadger left a comment

Choose a reason for hiding this comment

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

This would basically mean we have 2 versions of the version command. I dont think we should do this. Maybe we could talk about it.

Copy link
Contributor

@didroe didroe left a comment

Choose a reason for hiding this comment

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

❤️

@cfabianski
Copy link
Collaborator Author

This would basically mean we have 2 versions of the version command. I dont think we should do this. Maybe we could talk about it.

Well I agree it's not ideal to have 2 ways to show the version. IMO the bearer version isn't good / standard (I often got caught by it myself). It only exists because we had not implemented it correctly in the first place.
The only reason I hadn't removed the previous way as part of this PR was to be backward compatible but I would have removed it. Do you think I should do it?

@gotbadger
Copy link
Contributor

Bearer command structure is modelled after tools like docker which use this patten. So its not right to say its implmented incorrectly. It just is implemented like that :)

@gotbadger gotbadger dismissed their stale review November 6, 2023 13:50

Guess its not harmful so up to you

@cfabianski cfabianski merged commit 672abc5 into main Nov 6, 2023
6 checks passed
@cfabianski cfabianski deleted the chore/set-version-to-app branch November 6, 2023 14:18
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