NotificationsYou must be signed in to change notification settings
Fork927
Star10.1k

feat: refactor deployment config#6347

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

ammario merged 89 commits intomainfrombigcli

Mar 7, 2023

Merged

feat: refactor deployment config#6347

ammario merged 89 commits intomainfrombigcli

Mar 7, 2023

Conversation

Copy link

Member

ammario commentedFeb 25, 2023•
edited
Loading

This PR introduces a newbigcli package and refactorscoder server to 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 thecoder CLI 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:

Removed long-deprecated Experimental flag

TODO

Support Git Auth
Manually confirm that environment variable and flag names match old implementation

Follow-on issues:

High-priority-cleanup

Generate CLI docs again
Convert the rest of the CLI to bigcli
Refactor rest ofcli/server_test.go to use "clitest.Start" for better cleanup and error reporting
Regenerate golden files

Other

Document / Validate YAML works
Send back telemetry for each server option, plus how they're set. This makes it easier to make / communicate breaking changes.
Document how Support Links work in the YAML
Generate all Deployment setting pages and navigation from Options array

ammario added5 commits

February 25, 2023 22:48

Initial scaffold

6c29207

Add Duration

12dcc45

Begin converting AccessURL

d1e2e15

fixup! Begin converting AccessURL

8db4626

Start migrating Server

dc81f47

github-advanced-securitybot found potential problems

Feb 26, 2023

View reviewed changes

cli/bigcli/value.go FixedShow fixedHide fixed

ammario added24 commits

February 26, 2023 00:50

Convert a couple more fields

d511001

replace DeploymentConfig[T] with bigcli.[T]

8cc406e

Remove .Value calls

18b2552

Finish destroying newConfig

bad7aca

delete deployment/

f075ad5

rewrite server.go

69600d7

ugh

63159a6

Server boots

03b08fa

Move critical options code intocodersdk

e0d6c1e

DefaultCacheDir

5bebd0f

Merge remote-tracking branch 'origin/main' into bigcli

2c7b39d

Fix config dir bug

05b3a10

update golden --help

dcbd0d8

Support NoOptDefValue

fc06f64

Don't hide pflag.FlagSet

d31fa37

Help template scaffold

0d7d557

Show env in help output

f3d45d3

Merge remote-tracking branch 'origin/main' into bigcli

d322114

Write more descriptions

8f9d44b

Organize telemetry

acac92d

Finish grouping up flags

1c8359b

Render subcommands

8d0020f

Convert Groups to first class bigcli feature

882a80f

deepMapNode?

ae858b4

ammario added4 commits

March 1, 2023 23:17

Re-enable cli docs gen

74c6da2

.. Just not for server

De-pointer DeploymentValues

9e53c96

Decompose DeploymentConfig

06f82b5

Some comments

2f5a522

mafredri reviewed

Mar 6, 2023

View reviewed changes

Copy link

Member

mafredri left a comment

Choose a reason for hiding this comment

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

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 itbig? 😆.

cli/bigcli/bigcli.go OutdatedShow resolvedHide resolved

cli/bigcli/command.go Outdated

		return strings.Join(names, " ")
		}

		// FullName returns usage of the command, preceded

Copy link

Member

mafredriMar 6, 2023

Choose a reason for hiding this comment

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

Suggested change

	//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 {

Copy link

Member

mafredriMar 6, 2023

Choose a reason for hiding this comment

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

When do we need to show parent command usage in addition to current command usage?

Copy link

MemberAuthor

ammarioMar 6, 2023

Choose a reason for hiding this comment

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

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) {

Copy link

Member

mafredriMar 6, 2023

Choose a reason for hiding this comment

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

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

Copy link

Member

mafredriMar 6, 2023

Choose a reason for hiding this comment

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

Should we consider usingerrors.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.

cli/config/file.go OutdatedShow resolvedHide resolved

cli/usage.goShow resolvedHide resolved

cli/usage.tplShow resolvedHide resolved

coderd/users_test.go OutdatedShow resolvedHide resolved

codersdk/deployment.go OutdatedShow resolvedHide resolved

ammario added2 commits

March 6, 2023 18:03

Move envparse into bigcli

53e92fc

Minor changes

e9e2908

Copy link

MemberAuthor

ammario commentedMar 6, 2023•
edited
Loading

PS. I'm not sold on the name, what makes itbig? 😆.

Struggled to find a good name, butbig because it's meant to support bigger CLIs lol. If we think it's not very clear, I'm also down to use a more "cute" name likeserpent (following on the cobra/viper snake theme).

I'd like to see the re-introduction of golden files (for the server command) so that visual changes are always verified.

I added this as a high priority follow-up.

ammario added6 commits

March 6, 2023 19:03

Add context to clitest.Start

c3a11fd

Remove dead function

a090cd4

Merge remote-tracking branch 'origin/main' into bigcli

d9a8da6

Fix FE

e53367a

make lint passes

6252316

Pass highlyConfigurable

bf1a099

ammario requested a review fromkylecarbs

March 6, 2023 19:19

ammario added3 commits

March 6, 2023 19:23

Context bug

274d5c5

Rename to clibase

a213d25

fix slim build

2938980

ammario mentioned this pull request

Mar 7, 2023

feat: convert rest of CLI to clibase#6491

Merged

18 tasks

ammario added3 commits

March 7, 2023 18:27

Merge remote-tracking branch 'origin/main' into bigcli

140438e

Fix merge issues

46936f5

Set max token lifetime correctly

06277c4

cc@Kira-Pilot

kylecarbs approved these changes

Mar 7, 2023

View reviewed changes

Copy link

Member

kylecarbs left a comment

Choose a reason for hiding this comment

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

Some manual stuff testing:

^See the lack of space betweenrunning andCoder

^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.

cli/root_test.go OutdatedShow resolvedHide resolved

cli/clibase/bigcli.go OutdatedShow resolvedHide resolved

cli/clibase/env.goShow resolvedHide resolved

ammario added4 commits

March 7, 2023 20:40

Command -> Cmd

fc8e6ca

Typo

2a74445

Show disabled badge in zero option table

3cfb4bf

Merge remote-tracking branch 'origin/main' into bigcli

4d8bc75

ammario merged commit3b73321 intomain

Mar 7, 2023

ammario deleted the bigcli branch

March 7, 2023 21:10

github-actionsbot locked and limited conversation to collaborators

Mar 7, 2023

Labels

None yet

3 participants

Movatterモバイル変換

feat: refactor deployment config#6347

feat: refactor deployment config#6347

Uh oh!

Conversation

ammario commentedFeb 25, 2023• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

TODO

Follow-on issues:

High-priority-cleanup

Other

Uh oh!

Uh oh!

mafredri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mafredriMar 6, 2023

Choose a reason for hiding this comment

Uh oh!

mafredriMar 6, 2023

Choose a reason for hiding this comment

Uh oh!

ammarioMar 6, 2023

Choose a reason for hiding this comment

Uh oh!

mafredriMar 6, 2023

Choose a reason for hiding this comment

Uh oh!

mafredriMar 6, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ammario commentedMar 6, 2023• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

kylecarbs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ammario commentedFeb 25, 2023•
edited
Loading

ammario commentedMar 6, 2023•
edited
Loading