Skip to content

etcdctlv3: use spf13/cobra for cli interface#3922

Merged
xiang90 merged 2 commits intoetcd-io:masterfrom
gyuho:etcdctlv3_with_cobra
Nov 27, 2015
Merged

etcdctlv3: use spf13/cobra for cli interface#3922
xiang90 merged 2 commits intoetcd-io:masterfrom
gyuho:etcdctlv3_with_cobra

Conversation

@gyuho
Copy link
Contributor

@gyuho gyuho commented Nov 25, 2015

/cc @jonboulle @xiang90

Reference: https://github.com/coreos/rkt/blob/master/rkt/rkt.go

This replaces codegansta/cli with spf13/cobra according to
https://github.com/coreos/docs/blob/master/golang/README.md#cli.

Here's the sample output:

./etcdctlv3 -h
NAME:
    etcdctlv3 - A simple command line client for etcd3.

USAGE:
    etcdctlv3

VERSION:
    2.3.0-alpha.0+git

COMMANDS:
    range       Range gets the keys in the range from the store.
    put     Put puts the given key into the store.
    delete-range    DeleteRange deletes the given range from the store.
    txn     Txn processes all the requests in one transaction.
    compaction  Compaction compacts the event history in etcd.
    watch       Watch watches the events happening or happened.
    version     Print the version of etcd.
    help        Help about any command

OPTIONS:
      --endpoint="127.0.0.1:2378"   gRPC endpoint

@gyuho gyuho force-pushed the etcdctlv3_with_cobra branch 10 times, most recently from 817c6af to 67a6a94 Compare November 25, 2015 08:44
@philips
Copy link
Contributor

philips commented Nov 25, 2015

The biggest problem is we will break every single user that is using a
single - long arg vs a -- long arg. How do we fix that?

On Tue, Nov 24, 2015 at 10:12 PM Gyu-Ho Lee notifications@github.com
wrote:

updated error handling like etcdctl package:

fmt.Fprintln(os.Stderr, err.Error())
os.Exit(ExitCode)


Reply to this email directly or view it on GitHub
#3922 (comment).

@gyuho
Copy link
Contributor Author

gyuho commented Nov 25, 2015

@philips good point. I was thinking only replacing the etcdctl* with cobra because they are already in codegansta/cli which uses --long and -short style.

Etcdmain is written with standard flag package and supports both single and double dash, so there is no reason to change that I think.

Thanks!

@philips
Copy link
Contributor

philips commented Nov 25, 2015

I don't mind changing libraries, we just can't break etcd's flags.

On Wed, Nov 25, 2015 at 7:30 AM Gyu-Ho Lee notifications@github.com wrote:

@philips https://github.com/philips good point. I was thinking only
replacing the etcdctl* with cobra because they are already in
codegansta/cli which uses --long and -short style.

Etcdmain is written with standard flag package and supports both single
and double dash, so there is no reason to change that I think.

Thanks!


Reply to this email directly or view it on GitHub
#3922 (comment).

@xiang90
Copy link
Contributor

xiang90 commented Nov 25, 2015

@gyuho @philips This is just for etcdctlv3, right? We can try whatever we want there at least for now... But what is the motivation for this experiment?

@gyuho
Copy link
Contributor Author

gyuho commented Nov 25, 2015

@xiang90 yeah this is ONLY for etcdctl packages. And this is partially for #3901. I have code for etcdctl as well but wanted this approved first. Thanks!

@xiang90
Copy link
Contributor

xiang90 commented Nov 25, 2015

@gyuho Well... There is also the current etcdctl not only etcdctlv3

@gyuho
Copy link
Contributor Author

gyuho commented Nov 25, 2015

You are right. I was going to split the PRs because etcdctl is much longer. I think if we switch, we can finish migrating etcdctlv3 with godep and work on etcdctl.

What do you think?

@xiang90
Copy link
Contributor

xiang90 commented Nov 25, 2015

@gyuho For #3901, we can fix the cli lib we are using. Switching to cobra breaks some flag convention for etcdctl if I remember correctly.

@gyuho
Copy link
Contributor Author

gyuho commented Nov 25, 2015

Fair enough. I don't mind dropping this PR :)
Xiang do you remember what exactly was breaking with cobra? Flag parsing is straightforward so it shouldn't break anything I believe. Thanks!

@jonboulle
Copy link
Contributor

We're not concerned about breaking etcdctlv3 CLI compatibility are we?

@xiang90
Copy link
Contributor

xiang90 commented Nov 25, 2015

@jonboulle no we are not

@gyuho check this #2892 (comment)

@xiang90
Copy link
Contributor

xiang90 commented Nov 25, 2015

@gyuho OK. So probably here is what we should do:

keep etcdctl the same.
Use cobra for future command line tools: etcdctl3, etcdbench.

Copy link
Contributor

Choose a reason for hiding this comment

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

godep this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. Thanks!

@gyuho gyuho force-pushed the etcdctlv3_with_cobra branch from 67a6a94 to e18d083 Compare November 25, 2015 20:41
@gyuho
Copy link
Contributor Author

gyuho commented Nov 25, 2015

@xiang90 I fixed as you suggested with godep dependencies.

Please let me know if you have any feedback. Happy Thanksgiving all!

@xiang90
Copy link
Contributor

xiang90 commented Nov 25, 2015

@gyuho We need to separate the godep to another commit for easy review.

@gyuho
Copy link
Contributor Author

gyuho commented Nov 25, 2015

Sure! I will separate when I get home. And cobra supports string slice.
Should we chnage?

Sincerely,
Gyu-Ho Lee
On Nov 25, 2015 12:46, "Xiang Li" notifications@github.com wrote:

@gyuho https://github.com/gyuho We need to separate the godep to
another commit for easy review.


Reply to this email directly or view it on GitHub
#3922 (comment).

@xiang90
Copy link
Contributor

xiang90 commented Nov 25, 2015

@gyuho Do not worry. Take your time. Ideally we can make the change. But we can do it in another pr too.

@gyuho gyuho force-pushed the etcdctlv3_with_cobra branch from e18d083 to 8393a6c Compare November 25, 2015 22:09
@gyuho
Copy link
Contributor Author

gyuho commented Nov 25, 2015

@xiang90 Just split this into two commits. Please take a look again.

Agree. We can migrate this to cobra first and then figure out how to handle endpoint
in string or string slice.

Thanks!

@gyuho gyuho force-pushed the etcdctlv3_with_cobra branch 4 times, most recently from c8d5c18 to b37ebba Compare November 25, 2015 23:07
@gyuho gyuho force-pushed the etcdctlv3_with_cobra branch from b37ebba to b7647e0 Compare November 26, 2015 16:01
@gyuho
Copy link
Contributor Author

gyuho commented Nov 26, 2015

@xiang90 Thanks for review.

I pushed some updates:

  1. Remove ErrorTxnMalformed and changed it to ErrorInvalidInput
  2. Improve error message for bad arguments
  3. Fixes some wrong error code

Please take a look again. And let me know if you find anything wrong.

@xiang90
Copy link
Contributor

xiang90 commented Nov 26, 2015

LGTM

@gyuho
Copy link
Contributor Author

gyuho commented Nov 26, 2015

Thanks!

xiang90 added a commit that referenced this pull request Nov 27, 2015
etcdctlv3: use spf13/cobra for cli interface
@xiang90 xiang90 merged commit d705df0 into etcd-io:master Nov 27, 2015
@gyuho gyuho deleted the etcdctlv3_with_cobra branch January 31, 2016 00:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants