feat: refactor deployment config #6347
Conversation
.. Just not for server
There was a problem hiding this comment.
I haven't taken a look at the actual CLI output, but so far this seems like an improvement over what we have. I'd like to see the re-introduction of golden files (for the server command) so that visual changes are always verified.
It looks like a few things are still missing for this to be mergeable, so just leaving comments for now.
PS. I'm not sold on the name, what makes it big? 😆.
cli/bigcli/command.go
Outdated
| return strings.Join(names, " ") | ||
| } | ||
|
|
||
| // FullName returns usage of the command, preceded |
There was a problem hiding this comment.
| // FullName returns usage of the command, preceded | |
| // FullUsage returns usage of the command, preceded |
cli/bigcli/command.go
Outdated
|
|
||
| // FullName returns usage of the command, preceded | ||
| // by the usage of its parents. | ||
| func (c *Command) FullUsage() string { |
There was a problem hiding this comment.
When do we need to show parent command usage in addition to current command usage?
There was a problem hiding this comment.
We do so in the help template, perhaps not validly.
cli/bigcli/option.go
Outdated
| type OptionSet []Option | ||
|
|
||
| // Add adds the given Options to the OptionSet. | ||
| func (s *OptionSet) Add(opts ...Option) { |
There was a problem hiding this comment.
Consider updating function signature, since this is a set, we should do some verification/erroring here. Alternatively we may consider renaming, whichever makes more sense.
cli/bigcli/option.go
Outdated
|
|
||
| // ParseEnv parses the given environment variables into the OptionSet. | ||
| func (s *OptionSet) ParseEnv(globalPrefix string, environ []string) error { | ||
| var merr *multierror.Error |
There was a problem hiding this comment.
Should we consider using errors.Join instead? (I see we have one or two other uses of multierr currently, but we could nip that in the bud unless they give us something we need.
Struggled to find a good name, but
I added this as a high priority follow-up. |
There was a problem hiding this comment.
Some manual stuff testing:
^See the lack of space between running and Coder
^It says "Git Authentication Providers" randomly
^It's worth manually checking if this is still working by default. I know the value was false before, but somehow internally I think it was actually true.
This is in the deployment settings.
This PR introduces a new
bigclipackage and refactorscoder serverto use it instead of cobra/viper. It enables non-janky long-help formatting, doc generation, and YAML support. As a follow-up, I will convert the rest of thecoderCLI to usebigcli. After a month or two of stability, we can open sourcebigcli, probably replacing the defunctcoder/cli.The line of codes should be around net zero, but the new code will be much easier to understand, maintain, and extend.
Breaking changes:
TODO
Follow-on issues:
High-priority-cleanup
cli/server_test.goto use "clitest.Start" for better cleanup and error reportingOther