make gen + self review cleanup

coder · ammario · Apr 7, 2023 · Mar 30, 2023 · Mar 30, 2023 · Mar 30, 2023
commit 64255b39668ad5ec3ae5662a57186143de1de2f8
diff --git a/cli/clibase/cmd.go b/cli/clibase/cmd.go
@@ -270,31 +270,27 @@ func (inv *Invocation) run(state *runState) error {
 		}
 	}
 
-	// Read configs, if any.
+	// Read YAML configs, if any.
 	for _, opt := range inv.Command.Options {
 		path, ok := opt.Value.(*YAMLConfigPath)
 		if !ok || path.String() == "" {
 			continue
 		}
 
-		fi, err := os.OpenFile(path.String(), os.O_RDONLY, 0)
+		byt, err := os.ReadFile(path.String())
 		if err != nil {
-			return xerrors.Errorf("opening config file: %w", err)
+			return xerrors.Errorf("reading yaml: %w", err)
 		}
-		//nolint:revive
-		defer fi.Close()
-
-		dec := yaml.NewDecoder(fi)
 
 		var n yaml.Node
-		err = dec.Decode(&n)
+		err = yaml.Unmarshal(byt, &n)
 		if err != nil {
-			return xerrors.Errorf("decoding config: %w", err)
+			return xerrors.Errorf("decoding yaml: %w", err)
 		}
 
 		err = inv.Command.Options.UnmarshalYAML(&n)
 		if err != nil {
-			return xerrors.Errorf("applying config: %w", err)
+			return xerrors.Errorf("applying yaml: %w", err)
 		}
 	}
 

diff --git a/cli/clibase/values.go b/cli/clibase/values.go
@@ -351,8 +351,9 @@ func (s *Struct[T]) MarshalYAML() (interface{}, error) {
 }
 
 func (s *Struct[T]) UnmarshalYAML(n *yaml.Node) error {
-	// HACK: for compatibility with flags, we set the value to nil if the node
-	// is empty and T is a slice.
+	// HACK: for compatibility with flags, we use nil slices instead of empty
+	// slices. In most cases, nil slices and empty slices are treated
+	// the same, so this behavior may be removed at some point.
 	if typ := reflect.TypeOf(s.Value); typ.Kind() == reflect.Slice && len(n.Content) == 0 {
 		reflect.ValueOf(&s.Value).Elem().Set(reflect.Zero(typ))
 		return nil

diff --git a/cli/clibase/yaml.go b/cli/clibase/yaml.go
@@ -150,8 +150,8 @@ func (s *OptionSet) MarshalYAML() (any, error) {
 	return &root, nil
 }
 
-// mapYAMLNodes converts n into a map with keys of form "group.subgroup.option"
-// and values of the corresponding YAML nodes.
+// mapYAMLNodes converts parent into a map with keys of form "group.subgroup.option"
+// and values as the corresponding YAML nodes.
 func mapYAMLNodes(parent *yaml.Node) (map[string]*yaml.Node, error) {
 	if parent.Kind != yaml.MappingNode {
 		return nil, xerrors.Errorf("expected mapping node, got type %v", parent.Kind)
@@ -161,30 +161,35 @@ func mapYAMLNodes(parent *yaml.Node) (map[string]*yaml.Node, error) {
 	}
 	var (
 		key  string
-		m    = make(map[string]*yaml.Node)
+		m    = make(map[string]*yaml.Node, len(parent.Content)/2)
 		merr error
 	)
 	for i, child := range parent.Content {
 		if i%2 == 0 {
 			if child.Kind != yaml.ScalarNode {
+				// We immediately because the rest of the code is bound to fail
+				// if we don't know to expect a key or a value.
 				return nil, xerrors.Errorf("expected scalar node for key, got type %v", child.Kind)
 			}
 			key = child.Value
 			continue
 		}
-		// Even if we have a mapping node, we don't know if it's a grouped simple
-		// option a complex option, so we store both "key" and "group.key". Since
-		// we're storing pointers, the additional memory is of little concern.
+
+		// We don't know if this is a grouped simple option or complex option,
+		// so we store both "key" and "group.key". Since we're storing pointers,
+		// the additional memory is of little concern.
 		m[key] = child
-		if child.Kind == yaml.MappingNode {
-			sub, err := mapYAMLNodes(child)
-			if err != nil {
-				merr = errors.Join(merr, xerrors.Errorf("mapping node %q: %w", key, err))
-				continue
-			}
-			for k, v := range sub {
-				m[key+"."+k] = v
-			}
+		if child.Kind != yaml.MappingNode {
+			continue
+		}
+
+		sub, err := mapYAMLNodes(child)
+		if err != nil {
+			merr = errors.Join(merr, xerrors.Errorf("mapping node %q: %w", key, err))
+			continue
+		}
+		for k, v := range sub {
+			m[key+"."+k] = v
 		}
 	}
 
@@ -209,7 +214,7 @@ func (o *Option) setFromYAMLNode(n *yaml.Node) error {
 		}
 		return n.Decode(o.Value)
 	case yaml.MappingNode:
-		return xerrors.Errorf("mapping node must implement yaml.Unmarshaler")
+		return xerrors.Errorf("mapping nodes must implement yaml.Unmarshaler")
 	default:
 		return xerrors.Errorf("unexpected node kind %v", n.Kind)
 	}
@@ -218,8 +223,8 @@ func (o *Option) setFromYAMLNode(n *yaml.Node) error {
 // UnmarshalYAML converts the given YAML node into the option set.
 // It is isomorphic with ToYAML.
 func (s *OptionSet) UnmarshalYAML(rootNode *yaml.Node) error {
-	// The rootNode will be a DocumentNode if it's read from a file. Currently,
-	// we don't support multiple YAML documents.
+	// The rootNode will be a DocumentNode if it's read from a file. We do
+	// not support multiple documents in a single file.
 	if rootNode.Kind == yaml.DocumentNode {
 		if len(rootNode.Content) != 1 {
 			return xerrors.Errorf("expected one node in document, got %d", len(rootNode.Content))

diff --git a/cli/clitest/clitest.go b/cli/clitest/clitest.go
@@ -226,7 +226,7 @@ func StartWithWaiter(t *testing.T, inv *clibase.Invocation) *ErrorWaiter {
 			// down Postgres.
 			t.Logf("command %q timed out during test cleanup", inv.Command.FullName())
 		}
-		// When or not this failed the test is up to the caller.
+		// Whether or not this fails the test is left to the caller.
 		t.Logf("command %q exited with error: %v", inv.Command.FullName(), err)
 		errCh <- err
 	}()

diff --git a/cli/testdata/coder_server_--help.golden b/cli/testdata/coder_server_--help.golden
@@ -60,9 +60,12 @@ Clients include the coder cli, vs code extension, and the web UI.
 [1mConfig Options[0m 
 Use a YAML configuration file when your server launch become unwieldy.
 
-      --write-config bool, $CODER_WRITE_CONFIG
+  -c, --config yaml-config-path, $CODER_CONFIG_PATH
+          Specify a YAML file to load configuration from.
+
+      --write-config bool
 
-          Write out the current server as YAML to stdout.
+          Write out the current server config as YAML to stdout.
 
 [1mIntrospection / Logging Options[0m 
       --log-human string, $CODER_LOGGING_HUMAN (default: /dev/stderr)

diff --git a/cli/testdata/server-config.yaml.golden b/cli/testdata/server-config.yaml.golden
@@ -202,6 +202,9 @@ oidc:
     # {"access_type": "offline"}, type: struct[map[string]string])
     authURLParams:
         access_type: offline
+    # Ignore the userinfo endpoint and only use the ID token for user information.
+    # (default: false, type: bool)
+    ignoreUserInfo: false
     # Change the OIDC default 'groups' claim field. By default, will be 'groups' if
     # present in the oidc scopes argument. (default: <unset>, type: string)
     groupField: ""

diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go
diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json
diff --git a/docs/api/general.md b/docs/api/general.md
@@ -371,19 +371,9 @@ curl -X GET http://coder-server:8080/api/v2/deployment/config \
       "flag": "string",
       "flag_shorthand": "string",
       "group": {
-        "children": [
-          {
-            "children": [],
-            "description": "string",
-            "name": "string",
-            "parent": {},
-            "yaml_name": "string"
-          }
-        ],
         "description": "string",
         "name": "string",
         "parent": {
-          "children": [{}],
           "description": "string",
           "name": "string",
           "parent": {},

diff --git a/docs/api/schemas.md b/docs/api/schemas.md
@@ -384,19 +384,9 @@
 
 ```json
 {
-  "children": [
-    {
-      "children": [],
-      "description": "string",
-      "name": "string",
-      "parent": {},
-      "yaml_name": "string"
-    }
-  ],
   "description": "string",
   "name": "string",
   "parent": {
-    "children": [{}],
     "description": "string",
     "name": "string",
     "parent": {},
@@ -408,13 +398,12 @@
 
 ### Properties
 
-| Name          | Type                                    | Required | Restrictions | Description |
-| ------------- | --------------------------------------- | -------- | ------------ | ----------- |
-| `children`    | array of [clibase.Group](#clibasegroup) | false    |              |             |
-| `description` | string                                  | false    |              |             |
-| `name`        | string                                  | false    |              |             |
-| `parent`      | [clibase.Group](#clibasegroup)          | false    |              |             |
-| `yaml_name`   | string                                  | false    |              |             |
+| Name          | Type                           | Required | Restrictions | Description |
+| ------------- | ------------------------------ | -------- | ------------ | ----------- |
+| `description` | string                         | false    |              |             |
+| `name`        | string                         | false    |              |             |
+| `parent`      | [clibase.Group](#clibasegroup) | false    |              |             |
+| `yaml_name`   | string                         | false    |              |             |
 
 ## clibase.HostPort
 
@@ -446,19 +435,9 @@
   "flag": "string",
   "flag_shorthand": "string",
   "group": {
-    "children": [
-      {
-        "children": [],
-        "description": "string",
-        "name": "string",
-        "parent": {},
-        "yaml_name": "string"
-      }
-    ],
     "description": "string",
     "name": "string",
     "parent": {
-      "children": [{}],
       "description": "string",
       "name": "string",
       "parent": {},
@@ -480,19 +459,9 @@
       "flag": "string",
       "flag_shorthand": "string",
       "group": {
-        "children": [
-          {
-            "children": [],
-            "description": "string",
-            "name": "string",
-            "parent": {},
-            "yaml_name": "string"
-          }
-        ],
         "description": "string",
         "name": "string",
         "parent": {
-          "children": [{}],
           "description": "string",
           "name": "string",
           "parent": {},
@@ -2028,19 +1997,9 @@ CreateParameterRequest is a structure used to create a new parameter value for a
       "flag": "string",
       "flag_shorthand": "string",
       "group": {
-        "children": [
-          {
-            "children": [],
-            "description": "string",
-            "name": "string",
-            "parent": {},
-            "yaml_name": "string"
-          }
-        ],
         "description": "string",
         "name": "string",
         "parent": {
-          "children": [{}],
           "description": "string",
           "name": "string",
           "parent": {},

diff --git a/docs/cli/server.md b/docs/cli/server.md
@@ -48,6 +48,15 @@ Whether Coder only allows connections to workspaces via the browser.
 
 The directory to cache temporary files. If unspecified and $CACHE_DIRECTORY is set, it will be used for compatibility with systemd.
 
+### -c, --config
+
+|             |                                 |
+| ----------- | ------------------------------- |
+| Type        | <code>yaml-config-path</code>   |
+| Environment | <code>$CODER_CONFIG_PATH</code> |
+
+Specify a YAML file to load configuration from.
+
 ### --dangerous-allow-path-app-sharing
 
 |             |                                                      |
@@ -790,3 +799,11 @@ Output debug-level logs.
 | Environment | <code>$CODER_WILDCARD_ACCESS_URL</code> |
 
 Specifies the wildcard hostname to use for workspace applications in the form "\*.example.com".
+
+### --write-config
+
+|      |                   |
+| ---- | ----------------- |
+| Type | <code>bool</code> |
+
+<br/>Write out the current server config as YAML to stdout.