-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[sg] share runner implementation between run and start #61161
Conversation
Co-authored-by: William Bezuidenhout <[email protected]>
…ph into jsm/sg-docker-cmds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I must say I'm not very up-to-date on the run
/start
internals these days, so review is very cursory 😛 Overall, just a single mechanism (at least internally) makes sense to me
I also added a flag to sg start to run commands directly so because sg start has more flags and features built in, and also it seems silly to have two commands that do the same thing. I'd love to just make sg run and alias for sg start --commands or something.
I think deprecating run
entirely makes sense too, sg start -c foobar
is nice
dev/sg/sg_start.go
Outdated
@@ -94,7 +98,10 @@ sg start -describe single-program | |||
Name: "profile", | |||
Usage: "Starts up pprof on port 6060", | |||
}, | |||
|
|||
&cli.BoolFlag{ | |||
Name: "commands", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Name: "commands", | |
Name: "commands", | |
Aliases: []string{"c"} | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should also update BashComplete
to work with -c
, otherwise the completion will suggest commandsets still when you actually want commands
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately "-c" is already being used by "--critical" debug level. Either we could change that or do you have a better name for --commands
? --services
and '-s'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm maybe -cmd
as an alias?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bobheadxi Okay this ended being a HUGE rabbit hole of working on the bash completions. The bad news is that I don't have it working here, but the semi-good news is that I do have this WIP PR that upgrades to v3 of CLI which seems to not have the crashing issues but does have some other known issues I'm tracking. I can keep chipping away at that and when it gets released we should get a suite of up updated bash command improvements.
Co-authored-by: Robert Lin <[email protected]>
Instead of putting any work into
sg run
I just created a shared runner betweensg run
andsg start
. I also added a flag tosg start
to run commands directly becausesg start
has more flags and features built in, and also it seems silly to have two commands that do the same thing. I'd love to just makesg run
and alias forsg start --commands
or something.Took a bunch of code to do this because the prior implementations were pretty fragile.
Test plan
Ran locally.