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 support for version command #741

Merged
merged 4 commits into from
Sep 30, 2021
Merged

Add support for version command #741

merged 4 commits into from
Sep 30, 2021

Conversation

lkysow
Copy link
Member

@lkysow lkysow commented Sep 23, 2021

How I've tested this PR:

  • compiled and run

How I expect reviewers to test this PR:

  • compile and run

Checklist:

  • Tests added: No tests added. This matches what we're doing in control-plane (no tests)
  • CHANGELOG entry added


var (
Copy link
Member Author

Choose a reason for hiding this comment

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

this code moved to version/version in top-level

@@ -12,7 +12,7 @@ type Command struct {
}

func (c *Command) Run(_ []string) int {
c.UI.Output(fmt.Sprintf("consul-k8s %s", c.Version))
c.UI.Output(fmt.Sprintf("consul-k8s-control-plane %s", c.Version))
Copy link
Member Author

Choose a reason for hiding this comment

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

fix missing change from monorepo move

@lkysow lkysow requested review from ndhanushkodi, a team and t-eckert and removed request for a team September 23, 2021 19:44
Copy link
Contributor

@ndhanushkodi ndhanushkodi 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 adding this, looks great!

We did have one changelog entry that went into the alpha release (only install was merged at the time) https://github.com/hashicorp/consul-k8s/blob/main/CHANGELOG.md#0340-september-17-2021

So this could be added to the changelog as support for version command.

I think the changelog is missing the uninstall command as well

}

func (c *Command) Synopsis() string {
return "Prints the version."
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return "Prints the version."
return "Prints the version of the CLI."

}

func (c *Command) Help() string {
return ""
Copy link
Contributor

Choose a reason for hiding this comment

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

Should there be something returned here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@t-eckert t-eckert left a comment

Choose a reason for hiding this comment

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

Just a few comments

- Also refactor the version code out into top-level version pkg to match
how the control-plane is factored
@lkysow lkysow merged commit ff27e8a into main Sep 30, 2021
@lkysow lkysow deleted the lkysow/cli-version branch September 30, 2021 20:10
lawliet89 pushed a commit to lawliet89/consul-k8s that referenced this pull request Oct 6, 2021
They never succeed because the Docker mirror is refusing our pulls.
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