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: version sub command remove --version and -v flag #2090

Merged
merged 6 commits into from Jun 6, 2022

Conversation

Emyrk
Copy link
Member

@Emyrk Emyrk commented Jun 6, 2022

What this does (#1543)

Removes coder -v and coder --version.

Now do coder version to see coder version.

$ coder version
Coder v0.0.0-devel+ae779c6 Mon Jun  6 15:18:00 UTC 2022
https://github.com/coder/coder/commit/ae779c6276a5f305273343c3d5d4836d6a16c890

cli/root.go Outdated Show resolved Hide resolved
Copy link
Member

@mafredri mafredri left a comment

Choose a reason for hiding this comment

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

Thanks for picking this one up! 👍🏻

cli/root.go Outdated Show resolved Hide resolved
cli/root.go Outdated
func versionCmd() *cobra.Command {
return &cobra.Command{
Use: "version",
Short: "Version for coder",
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: I think this reads a bit better, but up to you to include or not.

Suggested change
Short: "Version for coder",
Short: "Show coder version",

cli/root.go Outdated
Comment on lines 115 to 116
cmd.Flags().BoolP("placeholder", "v", false, "This flag is a placeholder to make the cobra version flag work appropriately")
_ = cmd.Flags().MarkHidden("placeholder")
Copy link
Member

Choose a reason for hiding this comment

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

Cobra doesn't add this flag if Version isn't specified in the root. I'd propose that being a cleaner alternative.

Having --version and coder version seems unnecessary to me. I'd prefer to have coder version.

Copy link
Member

Choose a reason for hiding this comment

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

I’m fine with only having version but having both is consistent with help (or do I misremember?) and I think covers a common use case. I.e. there’s a 50/50 chance someone will blindly do either --version or version instead of checking help 😄.

Copy link
Member

Choose a reason for hiding this comment

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

I like how Go just does go version. I haven't found it to be confusing tbh

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct cobra does not. However if we want to support --version, we would need a persistent pre-run function to support the flag. Cobra has some first class support for --version that works even if there are extra args, eg: coder --version workspaces.

I think it is reasonable to use their first class support for this reason.

As for --version or coder version. I don't see anything wrong with supporting both. It's nice as a user to do w/e is more familiar to you.

Maybe coder version can report more info like the version of the deployment you are logged into.

Examples:

  • git version & git --version
  • docker --version is less verbose than docker version
  • docker-compose --version is less verbose than docker-compose version

Copy link
Member

Choose a reason for hiding this comment

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

It's fine to support both, I don't really understand why we need to though. Seems like supporting coder version is fine, and supporting both seems to not really satisfy a need, but adds an additional user-contract for us to test.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll drop the flag for now then and just do coder version

@Emyrk Emyrk changed the title chore: version sub command and reserve -v flag on root. chore: version sub command remove --version and -v flag Jun 6, 2022
@Emyrk Emyrk requested review from kylecarbs and mafredri June 6, 2022 18:51
cli/root_test.go Outdated Show resolved Hide resolved
cli/root.go Show resolved Hide resolved
@Emyrk Emyrk merged commit e2b2580 into main Jun 6, 2022
@Emyrk Emyrk deleted the stevenmasley/version_cmd branch June 6, 2022 22:38
kylecarbs pushed a commit that referenced this pull request Jun 10, 2022
* test: Add unit test for version cmd
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.

None yet

3 participants