Skip to content
This repository has been archived by the owner on Jan 8, 2024. It is now read-only.

cli: pass raw string of server run flags to install command #2328

Merged
merged 10 commits into from
Sep 22, 2021

Conversation

krantzinator
Copy link
Contributor

@krantzinator krantzinator commented Sep 20, 2021

This allows users to pass flags "through" the install command to the server run command that all server install platforms use. Included in upgrade as well for Nomad and Docker.
There are a couple typos and doc updates for various items as well that I saw while moving around the install/run docs.

Fixes #1514
Fixes #1491
Augments #2325 by allowing the user to set their server log levels on an install.

To test:
Base case:

  • On main, run make, then use the built binary to install the Waypoint server on docker. Check out the logs (docker logs waypoint-server) and observe that there are INFO and DEBUG logs only.

Test case:

  • On this branch, run make, then use the built binary to install the Waypoint server on docker as follows: waypoint install -platform=docker -accept-tos -- -vvv. Check out the logs again, and observe there are now TRACE logs as well.

@krantzinator krantzinator added the pr/no-changelog No automatic changelog entry required for this pull request label Sep 20, 2021
@krantzinator krantzinator added this to the 0.6.0 milestone Sep 20, 2021
@krantzinator krantzinator requested a review from a team September 20, 2021 20:58
@krantzinator krantzinator removed the pr/no-changelog No automatic changelog entry required for this pull request label Sep 20, 2021
Copy link
Contributor

@mitchellh mitchellh left a comment

Choose a reason for hiding this comment

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

I wouldn't accept them as a string, because then we need to do shell splitting which is non-trivial to do. I noted some places in the PR where I expect it just doesn't work either if more than one argument is provided.

I would propose changing this to using -- as a breakpoint to start treating everything after that (the []string) as args to append onto the args list instead.

There are libraries to do shell splitting but the reason I generally don't recommend it is because I've done it at significant user-adoption-scale before with Vagrant and Packer and its a constant cat-and-mouse of edge cases to chase down. That's why I recommend anything that does subprocessing to force the user to enter it as a slice of arg strings rather than a shell string.

So my recommendation is to allow the user to do:

$ waypoint server install -platform=whatever -- -v -db=/foo

And the -v and -db=/foo get chosen out as the server flags. This is an idiomatic Unix-ism used by tools such as xargs which subprocess out to additional child processes, so this isn't some weird bespoke UX thing.

It'd be fairly easy to preprocess the args []string parameter on the install command to split by the first -- it sees, too.

internal/serverinstall/docker.go Outdated Show resolved Hide resolved
internal/cli/main.go Outdated Show resolved Hide resolved
internal/cli/main.go Outdated Show resolved Hide resolved
internal/cli/install.go Show resolved Hide resolved
Copy link
Member

@briancain briancain left a comment

Choose a reason for hiding this comment

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

👌🏻 nice! Love all the various website/docs fixups you found in this too. I didn't try to install on every single platform, but overall the PR looks good to me from a code perspective 🎉

.changelog/2328.txt Outdated Show resolved Hide resolved
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants