Use cobra to improve CLI #38

chadell · 2017-11-25T15:49:25Z

@dcaba I had to change "host" flag to "target", because "-h" is assigned to "help"

dcaba · 2017-12-02T12:38:43Z

README.md

  -s, --sleep int          Time you want to sleep between connections, in ms (default 10)
+  -t, --target string      [Required] Target host you want to open tcp connections against


do you really like target? For me a host is more concise; target is some kind of "generic", and someone may enter into confusion when specifying the format (host:port as the target value?). I see, in any case, the problem with the abbreviation of host (-h -> -help). Maybe we can just mimic the behaviour of nc or telnet; "destination port" could be the last mandatory argument (without a flag)

ack, see this commit 72fd4b1

dcaba · 2017-12-03T06:45:02Z

README.md

+tcpgoon tests concurrent connections towards a server listening on a TCP port
+
+Usage:
+  tcpgoon <host> <port> [flags]


flags after looks super strange! Can we move to tcpgoon [flags] ?

I will have a look, but it doesn't look to weird to me :)

I've changed in usage, but in the end it has no effect, you can put flags before or after arguments.

dcaba · 2017-12-03T06:55:34Z

I did a go get ./..., but go build was failing in the root directory (for me):

 % go build
# github.com/spf13/cobra
../../spf13/cobra/command.go:452:12: fs.ShorthandLookup undefined (type *pflag.FlagSet has no field or method ShorthandLookup)
../../spf13/cobra/command.go:1287:10: c.lflags.SortFlags undefined (type *pflag.FlagSet has no field or method SortFlags)
../../spf13/cobra/command.go:1287:32: c.Flags().SortFlags undefined (type *pflag.FlagSet has no field or method SortFlags)
../../spf13/cobra/command.go:1465:18: c.parentsPflags.SortFlags undefined (type *pflag.FlagSet has no field or method SortFlags)

I had to run go get -u github.com/spf13/pflag, as pointed by spf13/cobra#450 . Is it normal?

dcaba · 2017-12-03T06:56:12Z

cmd/root.go

@@ -0,0 +1,83 @@
+package cmd


why root.go and not main.go? What are you using as a reference for this directory/file structure?

It's the one recommended in Cobra readme ("Using the Cobra Library"), for the root command ("tcpgoon"). If we go for a subcommands mode, we will create specific files for each one.

I've rename root.go to tcpgoon.go as the main command. If we add more modes/subcommands later we will creates files for each one.

dcaba · 2017-12-03T07:18:27Z

cmd/root.go

+	return nil
+}
+
+func enableDebugging(params tcpgoonParams) {


its strange to have a function that wraps another one that looks to do the same. To make the differentiation obvious, I'd rename to enableDebuggingIfFlagSet

dcaba · 2017-12-03T07:21:37Z

cmd/tcpgoon.go

@@ -0,0 +1,24 @@
+package cmd


Now that we have this cmd package, not sure if having a separate cmdutil makes sense... what do you think?

mmm, this cmd package is focused on cobra commands/subcommands, and cmdutils are utils used there. We could merge them, but also keep separated.
As you prefer, we could merge them if it's clearer to you, I also think that reduce the number of packages makes it simpler.

Also, I would like to have only "command" files in package cmd, to make it easier to understand.

No problem then... i have some doubts the cmdutil is actually a good name (i don't want this to become a wildcard with no sense), but its true is quite isolated from the main() logic

dcaba · 2017-12-03T07:24:26Z

cmd/tcpgoon.go

+	"github.com/dachad/tcpgoon/tcpclient"
+)
+
+func run(params tcpgoonParams) {


if we ever have more than one command... whats your idea about the structure? I've been checking the skeleton https://github.com/tcnksm/gcli generates, and its quite nice (but does not support cobra). Maybe you want to take a look and see if this affects to the existence/name of this tcpgoon.go

My idea would be create new files for each subcommand and call them from the main.go, something like https://github.com/gohugoio/hugo/blob/master/commands/hugo.go#L185
and implement them like this https://github.com/gohugoio/hugo/blob/master/commands/server.go.
So, to make it more clearer I could rename root.go to tcpgoon.go and consolidate this root command...

dcaba · 2017-12-03T07:27:19Z

Hi! Take a look to the previous comments... most of them are just ideas, rather than change requirements (i was expecting your thoughts rather than immediate new commits :) . For instance, having a generic target is something you may have been able to defend, for instance, if we want to reuse the same field to other ways to indicate the target when we cover service discovery integration - but with this comment i don't expect you to rollback, just to think about it!!!). Good job Christian!

chadell · 2017-12-03T08:04:45Z

I haven't been able to reproduce the error with "go build"..., but I've seen teh reference you mentioned. It seems that also works in Travis.

dcaba · 2017-12-07T21:15:31Z

cmd/tcpgoon.go

+	params.hostPtr = args[0]
+	if port, err := strconv.Atoi(args[1]); err != nil && port <= 0 {
+		return errors.New("Port argument is not a valid integer")
+	} else {


Improvement captured by codacy: "if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)" -> this means you don't need the else block. Move params.portPtr = port after the if, as if the condition matches it means it will exit before reaching that point

ok, I've needed to move the strconv out of the if to make this possible, if not, a warning because port not assigned was raising

dcaba

See latest comments, but you can merge after minor changes / considering them

dcaba · 2017-12-07T21:23:19Z

@chadell , please, update also the dockerhub README with the updated flags!

chadell mentioned this pull request Nov 25, 2017

flags improvements #14

Closed

chadell requested a review from dcaba November 25, 2017 16:12

chadell assigned dcaba Nov 25, 2017

dcaba reviewed Dec 2, 2017

View reviewed changes

dcaba reviewed Dec 3, 2017

View reviewed changes

dcaba reviewed Dec 7, 2017

View reviewed changes

dcaba approved these changes Dec 7, 2017

View reviewed changes

chadell added 8 commits December 8, 2017 15:17

First try using cobra

7c2dd5f

Output usage when validation fails

d706ed5

Update Readme

5527d43

Simply the main run function moving all flags work in the PreRun

9b0b935

Move host/target and port to positional arguments instead of flags

6fdbb66

Improve readiness of cmd structure

8e5d4da

Move flags before arguments

29a2ba3

small refinement

3f2a9b7

chadell force-pushed the issue-14-flags branch from 8be632d to 3f2a9b7 Compare December 8, 2017 14:21

chadell merged commit 21ac799 into master Dec 8, 2017

chadell deleted the issue-14-flags branch December 8, 2017 14:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use cobra to improve CLI #38

Use cobra to improve CLI #38

chadell commented Nov 25, 2017 •

edited

Loading

dcaba Dec 2, 2017

chadell Dec 2, 2017

dcaba Dec 3, 2017

chadell Dec 3, 2017

chadell Dec 3, 2017

dcaba commented Dec 3, 2017

dcaba Dec 3, 2017

chadell Dec 3, 2017

chadell Dec 3, 2017

dcaba Dec 3, 2017

chadell Dec 3, 2017

dcaba Dec 3, 2017

chadell Dec 3, 2017 •

edited

Loading

chadell Dec 3, 2017

dcaba Dec 7, 2017

dcaba Dec 3, 2017

chadell Dec 3, 2017

dcaba Dec 7, 2017

dcaba commented Dec 3, 2017

chadell commented Dec 3, 2017

dcaba Dec 7, 2017

chadell Dec 8, 2017

dcaba left a comment

dcaba commented Dec 7, 2017

		-s, --sleep int Time you want to sleep between connections, in ms (default 10)
		-t, --target string [Required] Target host you want to open tcp connections against

Use cobra to improve CLI #38

Use cobra to improve CLI #38

Conversation

chadell commented Nov 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcaba commented Dec 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chadell Dec 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcaba commented Dec 3, 2017

chadell commented Dec 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcaba left a comment

Choose a reason for hiding this comment

dcaba commented Dec 7, 2017

chadell commented Nov 25, 2017 •

edited

Loading

chadell Dec 3, 2017 •

edited

Loading