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

[sg] share runner implementation between run and start #61161

Merged
merged 15 commits into from
Mar 19, 2024

Conversation

jamesmcnamara
Copy link
Contributor

@jamesmcnamara jamesmcnamara commented Mar 14, 2024

Instead of putting any work into sg run I just created a shared runner between sg run and sg start. I also added a flag to sg start to run commands directly 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.

Took a bunch of code to do this because the prior implementations were pretty fragile.

Test plan

Ran locally.

@cla-bot cla-bot bot added the cla-signed label Mar 14, 2024
Base automatically changed from jsm/sg-docker-cmds to main March 14, 2024 21:19
Copy link
Member

@bobheadxi bobheadxi left a 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 Show resolved Hide resolved
@@ -94,7 +98,10 @@ sg start -describe single-program
Name: "profile",
Usage: "Starts up pprof on port 6060",
},

&cli.BoolFlag{
Name: "commands",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Name: "commands",
Name: "commands",
Aliases: []string{"c"}

Copy link
Member

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

Copy link
Contributor Author

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'?

Copy link
Member

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?

Copy link
Contributor Author

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.

@jamesmcnamara jamesmcnamara changed the title Share runner implementation between sg run and sg start [sg] Share runner implementation between run and start Mar 18, 2024
@jamesmcnamara jamesmcnamara changed the title [sg] Share runner implementation between run and start [sg] share runner implementation between run and start Mar 18, 2024
@jamesmcnamara jamesmcnamara merged commit d5b72e8 into main Mar 19, 2024
11 checks passed
@jamesmcnamara jamesmcnamara deleted the jsm/sg-run-install branch March 19, 2024 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants