Skip to content
This repository has been archived by the owner on Jun 13, 2021. It is now read-only.

Remove version command, introduce --version option #619

Merged
merged 1 commit into from Sep 18, 2019

Conversation

ndeloof
Copy link
Contributor

@ndeloof ndeloof commented Sep 16, 2019

- What I did
Replaced version command with a --version option

- How I did it
Moved code around

- How to verify it
run docker app --version

- Description for the changelog
Remove version command, introduce --version option

- A picture of a cute animal (not mandatory but encouraged)
image

@@ -24,6 +27,12 @@ func NewRootCmd(use string, dockerCli command.Cli) *cobra.Command {
Use: use,
Annotations: map[string]string{"experimentalCLI": "true"},
RunE: func(cmd *cobra.Command, args []string) error {

Copy link
Member

Choose a reason for hiding this comment

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

nit: empty line

internal/version.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 16, 2019

Codecov Report

Merging #619 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #619   +/-   ##
=======================================
  Coverage   72.29%   72.29%           
=======================================
  Files          51       51           
  Lines        2660     2660           
=======================================
  Hits         1923     1923           
  Misses        497      497           
  Partials      240      240

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e495f22...3a79fe7. Read the comment docs.

@ndeloof ndeloof force-pushed the APP-213 branch 5 times, most recently from 34db651 to 28789cc Compare September 17, 2019 09:46
@@ -24,6 +27,11 @@ func NewRootCmd(use string, dockerCli command.Cli) *cobra.Command {
Use: use,
Annotations: map[string]string{"experimentalCLI": "true"},
RunE: func(cmd *cobra.Command, args []string) error {
if showversion {
Copy link
Member

Choose a reason for hiding this comment

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

nit: showVersion

Copy link
Member

@rumpl rumpl left a comment

Choose a reason for hiding this comment

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

Small nit but LGTM

@@ -24,6 +27,11 @@ func NewRootCmd(use string, dockerCli command.Cli) *cobra.Command {
Use: use,
Annotations: map[string]string{"experimentalCLI": "true"},
RunE: func(cmd *cobra.Command, args []string) error {
if showVersion:q {
Copy link
Member

Choose a reason for hiding this comment

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

:x is a shortcut for w+q in vim

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's pretend this never happened

@@ -20,9 +20,10 @@ func uninstallCmd(dockerCli command.Cli) *cobra.Command {
var opts uninstallOptions

cmd := &cobra.Command{
Use: "uninstall INSTALLATION_NAME [--target-context TARGET_CONTEXT] [OPTIONS]",
Use: "rm INSTALLATION_NAME [--target-context TARGET_CONTEXT] [OPTIONS]",
Copy link
Member

Choose a reason for hiding this comment

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

Was this meant to be pushed with the version change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

seems I messed up my branches :'(

@@ -20,9 +20,10 @@ func uninstallCmd(dockerCli command.Cli) *cobra.Command {
var opts uninstallOptions

cmd := &cobra.Command{
Use: "uninstall INSTALLATION_NAME [--target-context TARGET_CONTEXT] [OPTIONS]",
Copy link
Contributor

Choose a reason for hiding this comment

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

This hunk does not seem to be part of this PR

}

if Experimental == "on" {
res = append(res, fmt.Sprintf("Experimental: %s", Experimental))
Copy link
Member

Choose a reason for hiding this comment

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

It might be good to be more explicit here so that people aren't confused with the CLI experimental. Maybe Experimental app build?

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Copy link
Contributor

@jcsirot jcsirot left a comment

Choose a reason for hiding this comment

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

LGTM

@ndeloof ndeloof merged commit e45cba1 into docker:master Sep 18, 2019
@ndeloof ndeloof deleted the APP-213 branch September 18, 2019 14:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants