From ac997a9cddfdaa011b13e9b2779815338e152386 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 28 Sep 2023 15:49:11 -0500 Subject: [PATCH 01/20] chore: Return populated options vs a blank --- cli/server.go | 1 + coderd/coderd.go | 7 ++++- coderd/coderdtest/coderdtest.go | 48 +++++++++++++++++---------------- coderd/deployment.go | 2 +- 4 files changed, 33 insertions(+), 25 deletions(-) diff --git a/cli/server.go b/cli/server.go index 45d8d7dd471bd..0a1d19e15e5f3 100644 --- a/cli/server.go +++ b/cli/server.go @@ -617,6 +617,7 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd. MetricsCacheRefreshInterval: vals.MetricsCacheRefreshInterval.Value(), AgentStatsRefreshInterval: vals.AgentStatRefreshInterval.Value(), DeploymentValues: vals, + DeploymentOptions: opts, PrometheusRegistry: prometheus.NewRegistry(), APIRateLimit: int(vals.RateLimit.API.Value()), LoginRateLimit: loginRateLimit, diff --git a/coderd/coderd.go b/coderd/coderd.go index 24f0b38f08dc9..52e30ce55869f 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -40,6 +40,7 @@ import ( "cdr.dev/slog" "github.com/coder/coder/v2/buildinfo" + "github.com/coder/coder/v2/cli/clibase" "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/awsidentity" "github.com/coder/coder/v2/coderd/batchstats" @@ -152,7 +153,11 @@ type Options struct { MetricsCacheRefreshInterval time.Duration AgentStatsRefreshInterval time.Duration DeploymentValues *codersdk.DeploymentValues - UpdateCheckOptions *updatecheck.Options // Set non-nil to enable update checking. + // DeploymentOptions do contain the copy of DeploymentValues, but contain + // contextual information about how the values were set. + // Do not use DeploymentOptions to retrieve values, use DeploymentValues instead. + DeploymentOptions clibase.OptionSet + UpdateCheckOptions *updatecheck.Options // Set non-nil to enable update checking. // SSHConfig is the response clients use to configure config-ssh locally. SSHConfig codersdk.SSHConfigResponse diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index c91481d0d3a5c..20815741d649f 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -394,29 +394,31 @@ func NewOptions(t testing.TB, options *Options) (func(http.Handler), context.Can Pubsub: options.Pubsub, GitAuthConfigs: options.GitAuthConfigs, - Auditor: options.Auditor, - AWSCertificates: options.AWSCertificates, - AzureCertificates: options.AzureCertificates, - GithubOAuth2Config: options.GithubOAuth2Config, - RealIPConfig: options.RealIPConfig, - OIDCConfig: options.OIDCConfig, - GoogleTokenValidator: options.GoogleTokenValidator, - SSHKeygenAlgorithm: options.SSHKeygenAlgorithm, - DERPServer: derpServer, - APIRateLimit: options.APIRateLimit, - LoginRateLimit: options.LoginRateLimit, - FilesRateLimit: options.FilesRateLimit, - Authorizer: options.Authorizer, - Telemetry: telemetry.NewNoop(), - TemplateScheduleStore: &templateScheduleStore, - TLSCertificates: options.TLSCertificates, - TrialGenerator: options.TrialGenerator, - TailnetCoordinator: options.Coordinator, - BaseDERPMap: derpMap, - DERPMapUpdateFrequency: 150 * time.Millisecond, - MetricsCacheRefreshInterval: options.MetricsCacheRefreshInterval, - AgentStatsRefreshInterval: options.AgentStatsRefreshInterval, - DeploymentValues: options.DeploymentValues, + Auditor: options.Auditor, + AWSCertificates: options.AWSCertificates, + AzureCertificates: options.AzureCertificates, + GithubOAuth2Config: options.GithubOAuth2Config, + RealIPConfig: options.RealIPConfig, + OIDCConfig: options.OIDCConfig, + GoogleTokenValidator: options.GoogleTokenValidator, + SSHKeygenAlgorithm: options.SSHKeygenAlgorithm, + DERPServer: derpServer, + APIRateLimit: options.APIRateLimit, + LoginRateLimit: options.LoginRateLimit, + FilesRateLimit: options.FilesRateLimit, + Authorizer: options.Authorizer, + Telemetry: telemetry.NewNoop(), + TemplateScheduleStore: &templateScheduleStore, + TLSCertificates: options.TLSCertificates, + TrialGenerator: options.TrialGenerator, + TailnetCoordinator: options.Coordinator, + BaseDERPMap: derpMap, + DERPMapUpdateFrequency: 150 * time.Millisecond, + MetricsCacheRefreshInterval: options.MetricsCacheRefreshInterval, + AgentStatsRefreshInterval: options.AgentStatsRefreshInterval, + DeploymentValues: options.DeploymentValues, + // For tests, we don't have any `value_source` fields set. + DeploymentOptions: options.DeploymentValues.Options(), UpdateCheckOptions: options.UpdateCheckOptions, SwaggerEndpoint: options.SwaggerEndpoint, AppSecurityKey: AppSecurityKey, diff --git a/coderd/deployment.go b/coderd/deployment.go index 255f9c7ac2a8d..ebea5625583cd 100644 --- a/coderd/deployment.go +++ b/coderd/deployment.go @@ -33,7 +33,7 @@ func (api *API) deploymentValues(rw http.ResponseWriter, r *http.Request) { r.Context(), rw, http.StatusOK, codersdk.DeploymentConfig{ Values: values, - Options: values.Options(), + Options: api.DeploymentOptions, }, ) } From 37484309472f9557e125298807ac0b015c62b779 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 28 Sep 2023 15:59:10 -0500 Subject: [PATCH 02/20] Strip secret values --- cli/server.go | 5 ++++- coderd/coderd.go | 1 + codersdk/deployment.go | 14 ++++++++++++++ 3 files changed, 19 insertions(+), 1 deletion(-) diff --git a/cli/server.go b/cli/server.go index 0a1d19e15e5f3..37470bc283271 100644 --- a/cli/server.go +++ b/cli/server.go @@ -617,7 +617,10 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd. MetricsCacheRefreshInterval: vals.MetricsCacheRefreshInterval.Value(), AgentStatsRefreshInterval: vals.AgentStatRefreshInterval.Value(), DeploymentValues: vals, - DeploymentOptions: opts, + // Do not pass secret values to DeploymentOptions. All values should be read from + // the DeploymentValues instead, this just serves to indicate the source of each + // option. This is just defensive to prevent accidentally leaking. + DeploymentOptions: codersdk.DeploymentOptionsWithoutSecrets(opts), PrometheusRegistry: prometheus.NewRegistry(), APIRateLimit: int(vals.RateLimit.API.Value()), LoginRateLimit: loginRateLimit, diff --git a/coderd/coderd.go b/coderd/coderd.go index 52e30ce55869f..24dd9c7bfe540 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -156,6 +156,7 @@ type Options struct { // DeploymentOptions do contain the copy of DeploymentValues, but contain // contextual information about how the values were set. // Do not use DeploymentOptions to retrieve values, use DeploymentValues instead. + // All secrets values are stripped. DeploymentOptions clibase.OptionSet UpdateCheckOptions *updatecheck.Options // Set non-nil to enable update checking. diff --git a/codersdk/deployment.go b/codersdk/deployment.go index a13b4a1379fd6..8ce529cbedf47 100644 --- a/codersdk/deployment.go +++ b/codersdk/deployment.go @@ -1762,6 +1762,20 @@ type LinkConfig struct { Icon string `json:"icon" yaml:"icon"` } +// DeploymentOptionsWithoutSecrets returns a copy of the OptionSet with secret values omitted. +func DeploymentOptionsWithoutSecrets(set clibase.OptionSet) clibase.OptionSet { + cpy := make(clibase.OptionSet, 0, len(set)) + for _, opt := range set { + cpyOpt := opt + if IsSecretDeploymentOption(cpyOpt) { + var empty clibase.String + cpyOpt.Value = &empty + } + cpy = append(cpy, cpyOpt) + } + return cpy +} + // WithoutSecrets returns a copy of the config without secret values. func (c *DeploymentValues) WithoutSecrets() (*DeploymentValues, error) { var ff DeploymentValues From e1a899795d122ea6938f8d269b19d25dd23426c5 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 28 Sep 2023 16:24:33 -0500 Subject: [PATCH 03/20] Color the value source for a deployment value --- .../components/DeploySettingsLayout/Option.tsx | 16 +++++++++++----- .../DeploySettingsLayout/OptionsTable.tsx | 8 ++++---- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/site/src/components/DeploySettingsLayout/Option.tsx b/site/src/components/DeploySettingsLayout/Option.tsx index 427f8cf5632d2..6a4292d55b75d 100644 --- a/site/src/components/DeploySettingsLayout/Option.tsx +++ b/site/src/components/DeploySettingsLayout/Option.tsx @@ -58,21 +58,27 @@ export const OptionValue: FC<{ children?: unknown }> = ({ children }) => { return {JSON.stringify(children)}; }; -export const OptionConfig = (props: BoxProps) => { +// OptionalConfig takes a source bool to indicate if the Option is the source of the configured value. +export const OptionConfig = ({ + source, + ...boxProps +}: { source?: boolean } & BoxProps) => { return ( theme.palette.background.paperLight, + backgroundColor: (theme) => + source ? "green" : theme.palette.background.paperLight, display: "inline-flex", alignItems: "center", borderRadius: 0.25, padding: (theme) => theme.spacing(0, 1), - border: (theme) => `1px solid ${theme.palette.divider}`, - ...props.sx, + border: (theme) => + `1px solid ${source ? "lightgreen" : theme.palette.divider}`, + ...boxProps.sx, }} /> ); diff --git a/site/src/components/DeploySettingsLayout/OptionsTable.tsx b/site/src/components/DeploySettingsLayout/OptionsTable.tsx index d5a8cdad75b27..1f09a66c02b94 100644 --- a/site/src/components/DeploySettingsLayout/OptionsTable.tsx +++ b/site/src/components/DeploySettingsLayout/OptionsTable.tsx @@ -58,25 +58,25 @@ const OptionsTable: FC<{ }} > {option.flag && ( - + CLI --{option.flag} )} {option.flag_shorthand && ( - + CLI- {option.flag_shorthand} )} {option.env && ( - + ENV {option.env} )} {option.yaml && ( - + YAML {option.yaml} From a6cc10e7914c567b19e2edc862aaa09d51a19361 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 28 Sep 2023 16:29:27 -0500 Subject: [PATCH 04/20] Use theme colors --- site/src/components/DeploySettingsLayout/Option.tsx | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/site/src/components/DeploySettingsLayout/Option.tsx b/site/src/components/DeploySettingsLayout/Option.tsx index 6a4292d55b75d..8e0c9aceb6915 100644 --- a/site/src/components/DeploySettingsLayout/Option.tsx +++ b/site/src/components/DeploySettingsLayout/Option.tsx @@ -71,13 +71,17 @@ export const OptionConfig = ({ fontFamily: MONOSPACE_FONT_FAMILY, fontWeight: 600, backgroundColor: (theme) => - source ? "green" : theme.palette.background.paperLight, + source + ? theme.palette.success.dark + : theme.palette.background.paperLight, display: "inline-flex", alignItems: "center", borderRadius: 0.25, padding: (theme) => theme.spacing(0, 1), border: (theme) => - `1px solid ${source ? "lightgreen" : theme.palette.divider}`, + `1px solid ${ + source ? theme.palette.success.light : theme.palette.divider + }`, ...boxProps.sx, }} /> From be1aec3ebb78c0e2939009e6ff4c4a09c57c30ac Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 28 Sep 2023 16:37:22 -0500 Subject: [PATCH 05/20] try going with a blue --- site/src/components/DeploySettingsLayout/Option.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/site/src/components/DeploySettingsLayout/Option.tsx b/site/src/components/DeploySettingsLayout/Option.tsx index 8e0c9aceb6915..c3bb5e976bcaa 100644 --- a/site/src/components/DeploySettingsLayout/Option.tsx +++ b/site/src/components/DeploySettingsLayout/Option.tsx @@ -72,7 +72,7 @@ export const OptionConfig = ({ fontWeight: 600, backgroundColor: (theme) => source - ? theme.palette.success.dark + ? theme.palette.primary.dark : theme.palette.background.paperLight, display: "inline-flex", alignItems: "center", @@ -80,7 +80,7 @@ export const OptionConfig = ({ padding: (theme) => theme.spacing(0, 1), border: (theme) => `1px solid ${ - source ? theme.palette.success.light : theme.palette.divider + source ? theme.palette.primary.main : theme.palette.divider }`, ...boxProps.sx, }} From dc67bb7f4ff3f66458fc9410630f4991d0adec64 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 28 Sep 2023 16:45:27 -0500 Subject: [PATCH 06/20] Fix json marshal of secrets --- coderd/coderdtest/coderdtest.go | 49 ++++++++++++++++----------------- codersdk/deployment.go | 10 +++++-- 2 files changed, 32 insertions(+), 27 deletions(-) diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index 20815741d649f..e876480cde618 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -394,31 +394,30 @@ func NewOptions(t testing.TB, options *Options) (func(http.Handler), context.Can Pubsub: options.Pubsub, GitAuthConfigs: options.GitAuthConfigs, - Auditor: options.Auditor, - AWSCertificates: options.AWSCertificates, - AzureCertificates: options.AzureCertificates, - GithubOAuth2Config: options.GithubOAuth2Config, - RealIPConfig: options.RealIPConfig, - OIDCConfig: options.OIDCConfig, - GoogleTokenValidator: options.GoogleTokenValidator, - SSHKeygenAlgorithm: options.SSHKeygenAlgorithm, - DERPServer: derpServer, - APIRateLimit: options.APIRateLimit, - LoginRateLimit: options.LoginRateLimit, - FilesRateLimit: options.FilesRateLimit, - Authorizer: options.Authorizer, - Telemetry: telemetry.NewNoop(), - TemplateScheduleStore: &templateScheduleStore, - TLSCertificates: options.TLSCertificates, - TrialGenerator: options.TrialGenerator, - TailnetCoordinator: options.Coordinator, - BaseDERPMap: derpMap, - DERPMapUpdateFrequency: 150 * time.Millisecond, - MetricsCacheRefreshInterval: options.MetricsCacheRefreshInterval, - AgentStatsRefreshInterval: options.AgentStatsRefreshInterval, - DeploymentValues: options.DeploymentValues, - // For tests, we don't have any `value_source` fields set. - DeploymentOptions: options.DeploymentValues.Options(), + Auditor: options.Auditor, + AWSCertificates: options.AWSCertificates, + AzureCertificates: options.AzureCertificates, + GithubOAuth2Config: options.GithubOAuth2Config, + RealIPConfig: options.RealIPConfig, + OIDCConfig: options.OIDCConfig, + GoogleTokenValidator: options.GoogleTokenValidator, + SSHKeygenAlgorithm: options.SSHKeygenAlgorithm, + DERPServer: derpServer, + APIRateLimit: options.APIRateLimit, + LoginRateLimit: options.LoginRateLimit, + FilesRateLimit: options.FilesRateLimit, + Authorizer: options.Authorizer, + Telemetry: telemetry.NewNoop(), + TemplateScheduleStore: &templateScheduleStore, + TLSCertificates: options.TLSCertificates, + TrialGenerator: options.TrialGenerator, + TailnetCoordinator: options.Coordinator, + BaseDERPMap: derpMap, + DERPMapUpdateFrequency: 150 * time.Millisecond, + MetricsCacheRefreshInterval: options.MetricsCacheRefreshInterval, + AgentStatsRefreshInterval: options.AgentStatsRefreshInterval, + DeploymentValues: options.DeploymentValues, + DeploymentOptions: codersdk.DeploymentOptionsWithoutSecrets(options.DeploymentValues.Options()), UpdateCheckOptions: options.UpdateCheckOptions, SwaggerEndpoint: options.SwaggerEndpoint, AppSecurityKey: AppSecurityKey, diff --git a/codersdk/deployment.go b/codersdk/deployment.go index 8ce529cbedf47..3caf6e2fc9881 100644 --- a/codersdk/deployment.go +++ b/codersdk/deployment.go @@ -1768,8 +1768,14 @@ func DeploymentOptionsWithoutSecrets(set clibase.OptionSet) clibase.OptionSet { for _, opt := range set { cpyOpt := opt if IsSecretDeploymentOption(cpyOpt) { - var empty clibase.String - cpyOpt.Value = &empty + switch cpyOpt.Value.Type() { + case "string-array": + var empty clibase.StringArray + cpyOpt.Value = &empty + default: + var empty clibase.String + cpyOpt.Value = &empty + } } cpy = append(cpy, cpyOpt) } From 53b147bcefad8daccb9e25429c8ebc10da5ba8d2 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 28 Sep 2023 22:47:59 -0500 Subject: [PATCH 07/20] Fix unmarshal json for optionsets --- cli/clibase/option.go | 95 +++++++++++++++++++++++++++++++++++++++++++ cli/clibase/values.go | 26 ++++++++++++ cli/server_test.go | 16 ++++++++ 3 files changed, 137 insertions(+) diff --git a/cli/clibase/option.go b/cli/clibase/option.go index 8c7fed92e20f7..3e81020cdd438 100644 --- a/cli/clibase/option.go +++ b/cli/clibase/option.go @@ -1,6 +1,8 @@ package clibase import ( + "bytes" + "encoding/json" "os" "strings" @@ -65,6 +67,20 @@ type Option struct { ValueSource ValueSource `json:"value_source,omitempty"` } +// optionNoMethods is just a wrapper around Option so we can defer to the +// default json.Unmarshaler behavior. +type optionNoMethods Option + +func (o *Option) UnmarshalJSON(data []byte) error { + // If an option has no values, we have no idea how to unmarshal it. + // So just discard the json data. + if o.Value == nil { + o.Value = &DiscardValue + } + + return json.Unmarshal(data, (*optionNoMethods)(o)) +} + func (o Option) YAMLPath() string { if o.YAML == "" { return "" @@ -79,6 +95,85 @@ func (o Option) YAMLPath() string { // OptionSet is a group of options that can be applied to a command. type OptionSet []Option +// UnmarshalJSON implements json.Unmarshaler for OptionSets. Options have an +// interface Value type that cannot handle unmarshalling because the types cannot +// be inferred. Since it is a slice, instantiating the Options first does not +// help. +// +// However, we typically do instantiate the slice to have the correct types. +// So this unmarshaller will attempt to find the named option in the existing +// set, if it cannot, the value is discarded. If the option exists, the value +// is unmarshalled into the existing option, and replaces the existing option. +// +// The value is discarded if it's type cannot be inferred. This behavior just +// feels "safer", although it should never happen if the correct option set +// is passed in. +func (os *OptionSet) UnmarshalJSON(data []byte) error { + dec := json.NewDecoder(bytes.NewBuffer(data)) + // Should be a json array, so consume the starting open bracket. + t, err := dec.Token() + if err != nil { + return xerrors.Errorf("read array open bracket: %w", err) + } + if t != json.Delim('[') { + return xerrors.Errorf("expected array open bracket, got %q", t) + } + + // As long as json elements exist, consume them. The counter is used for + // better errors. + var i int +OptionSetDecodeLoop: + for dec.More() { + var opt Option + // jValue is a placeholder value that allows us to capture the + // raw json for the value to attempt to unmarshal later. + var jValue jsonValue + opt.Value = &jValue + err := dec.Decode(&opt) + if err != nil { + return xerrors.Errorf("decode %d option: %w", i, err) + } + + // Try to see if the option already exists in the option set. + // If it does, just update the existing option. + for i, have := range *os { + if have.Name == opt.Name { + if jValue != nil { + err := json.Unmarshal(jValue, &(*os)[i].Value) + if err != nil { + return xerrors.Errorf("decode option %q value: %w", have.Name, err) + } + // Set the opt's value + opt.Value = (*os)[i].Value + } else { + // Discard the value if it's nil. + opt.Value = DiscardValue + } + // Override the existing. + (*os)[i] = opt + // Go to the next option to decode. + continue OptionSetDecodeLoop + } + } + + // If the option doesn't exist, the value will be discarded. + // We do this because we cannot infer the type of the value. + opt.Value = DiscardValue + *os = append(*os, opt) + i++ + } + + t, err = dec.Token() + if err != nil { + return xerrors.Errorf("read array close bracket: %w", err) + } + if t != json.Delim(']') { + return xerrors.Errorf("expected array close bracket, got %q", t) + } + + return nil +} + // Add adds the given Options to the OptionSet. func (s *OptionSet) Add(opts ...Option) { *s = append(*s, opts...) diff --git a/cli/clibase/values.go b/cli/clibase/values.go index 6ec67d2d1bc09..4a13c9ff94549 100644 --- a/cli/clibase/values.go +++ b/cli/clibase/values.go @@ -430,6 +430,32 @@ func (discardValue) Type() string { return "discard" } +func (discardValue) UnmarshalJSON(data []byte) error { + return nil +} + +type jsonValue json.RawMessage + +func (jsonValue) Set(string) error { + return xerrors.Errorf("json value is read-only") +} + +func (jsonValue) String() string { + return "" +} + +func (jsonValue) Type() string { + return "json" +} + +func (j *jsonValue) UnmarshalJSON(data []byte) error { + if j == nil { + return xerrors.New("json.RawMessage: UnmarshalJSON on nil pointer") + } + *j = append((*j)[0:0], data...) + return nil +} + var _ pflag.Value = (*Enum)(nil) type Enum struct { diff --git a/cli/server_test.go b/cli/server_test.go index 49889d77c9540..ccb5730e52ce0 100644 --- a/cli/server_test.go +++ b/cli/server_test.go @@ -1188,6 +1188,22 @@ func TestServer(t *testing.T) { require.Equal(t, map[string]string{"serious_business_unit": "serious_business_unit"}, deploymentConfig.Values.OIDC.GroupMapping.Value) require.Equal(t, "Sign In With Coder", deploymentConfig.Values.OIDC.SignInText.Value()) require.Equal(t, "https://example.com/icon.png", deploymentConfig.Values.OIDC.IconURL.Value().String()) + + // Verify the option values + for _, opt := range deploymentConfig.Options { + switch opt.Flag { + case "access-url": + require.Equal(t, "http://example.com", opt.Value.String()) + case "oidc-icon-url": + require.Equal(t, "https://example.com/icon.png", opt.Value.String()) + case "oidc-sign-in-text": + require.Equal(t, "Sign In With Coder", opt.Value.String()) + case "redirect-to-access-url": + require.Equal(t, "false", opt.Value.String()) + case "derp-server-region-id": + require.Equal(t, "999", opt.Value.String()) + } + } }) }) From b1058a64bb926dbd9cd63c69661eacd0a9aac173 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 28 Sep 2023 22:54:44 -0500 Subject: [PATCH 08/20] Add comment --- cli/clibase/values.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/cli/clibase/values.go b/cli/clibase/values.go index 4a13c9ff94549..f4d7e523986b1 100644 --- a/cli/clibase/values.go +++ b/cli/clibase/values.go @@ -434,6 +434,9 @@ func (discardValue) UnmarshalJSON(data []byte) error { return nil } +// jsonValue is intentionally not exported. It is just used to store the raw JSON +// data for a value to defer it's unmarshal. It implements the pflag.Value to be +// usable in an Option. type jsonValue json.RawMessage func (jsonValue) Set(string) error { From 6ce3af7ef63c6d337d2e70a38ad6631fe424873f Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Fri, 29 Sep 2023 10:46:55 +0000 Subject: [PATCH 09/20] Add diff color to config option when it is source --- site/src/components/DeploySettingsLayout/Option.tsx | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/site/src/components/DeploySettingsLayout/Option.tsx b/site/src/components/DeploySettingsLayout/Option.tsx index c3bb5e976bcaa..4d9461b7fb415 100644 --- a/site/src/components/DeploySettingsLayout/Option.tsx +++ b/site/src/components/DeploySettingsLayout/Option.tsx @@ -3,6 +3,7 @@ import { PropsWithChildren, FC } from "react"; import { MONOSPACE_FONT_FAMILY } from "theme/constants"; import { DisabledBadge, EnabledBadge } from "./Badges"; import Box, { BoxProps } from "@mui/material/Box"; +import { combineClasses } from "utils/combineClasses"; export const OptionName: FC = ({ children }) => { const styles = useStyles(); @@ -82,6 +83,9 @@ export const OptionConfig = ({ `1px solid ${ source ? theme.palette.primary.main : theme.palette.divider }`, + "& .option-config-flag": { + backgroundColor: source ? "rgba(0, 0, 0, 0.7)" : undefined, + }, ...boxProps.sx, }} /> @@ -92,6 +96,7 @@ export const OptionConfigFlag = (props: BoxProps) => { return ( Date: Fri, 29 Sep 2023 08:49:27 -0500 Subject: [PATCH 10/20] rename optionset receiver --- cli/clibase/option.go | 48 +++++++++++++++++++++---------------------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/cli/clibase/option.go b/cli/clibase/option.go index 3e81020cdd438..2b84140ffdb8a 100644 --- a/cli/clibase/option.go +++ b/cli/clibase/option.go @@ -108,7 +108,7 @@ type OptionSet []Option // The value is discarded if it's type cannot be inferred. This behavior just // feels "safer", although it should never happen if the correct option set // is passed in. -func (os *OptionSet) UnmarshalJSON(data []byte) error { +func (optSet *OptionSet) UnmarshalJSON(data []byte) error { dec := json.NewDecoder(bytes.NewBuffer(data)) // Should be a json array, so consume the starting open bracket. t, err := dec.Token() @@ -136,21 +136,21 @@ OptionSetDecodeLoop: // Try to see if the option already exists in the option set. // If it does, just update the existing option. - for i, have := range *os { + for i, have := range *optSet { if have.Name == opt.Name { if jValue != nil { - err := json.Unmarshal(jValue, &(*os)[i].Value) + err := json.Unmarshal(jValue, &(*optSet)[i].Value) if err != nil { return xerrors.Errorf("decode option %q value: %w", have.Name, err) } // Set the opt's value - opt.Value = (*os)[i].Value + opt.Value = (*optSet)[i].Value } else { // Discard the value if it's nil. opt.Value = DiscardValue } // Override the existing. - (*os)[i] = opt + (*optSet)[i] = opt // Go to the next option to decode. continue OptionSetDecodeLoop } @@ -159,7 +159,7 @@ OptionSetDecodeLoop: // If the option doesn't exist, the value will be discarded. // We do this because we cannot infer the type of the value. opt.Value = DiscardValue - *os = append(*os, opt) + *optSet = append(*optSet, opt) i++ } @@ -175,14 +175,14 @@ OptionSetDecodeLoop: } // Add adds the given Options to the OptionSet. -func (s *OptionSet) Add(opts ...Option) { - *s = append(*s, opts...) +func (optSet *OptionSet) Add(opts ...Option) { + *optSet = append(*optSet, opts...) } // Filter will only return options that match the given filter. (return true) -func (s OptionSet) Filter(filter func(opt Option) bool) OptionSet { +func (optSet OptionSet) Filter(filter func(opt Option) bool) OptionSet { cpy := make(OptionSet, 0) - for _, opt := range s { + for _, opt := range optSet { if filter(opt) { cpy = append(cpy, opt) } @@ -191,13 +191,13 @@ func (s OptionSet) Filter(filter func(opt Option) bool) OptionSet { } // FlagSet returns a pflag.FlagSet for the OptionSet. -func (s *OptionSet) FlagSet() *pflag.FlagSet { - if s == nil { +func (optSet *OptionSet) FlagSet() *pflag.FlagSet { + if optSet == nil { return &pflag.FlagSet{} } fs := pflag.NewFlagSet("", pflag.ContinueOnError) - for _, opt := range *s { + for _, opt := range *optSet { if opt.Flag == "" { continue } @@ -234,8 +234,8 @@ func (s *OptionSet) FlagSet() *pflag.FlagSet { // ParseEnv parses the given environment variables into the OptionSet. // Use EnvsWithPrefix to filter out prefixes. -func (s *OptionSet) ParseEnv(vs []EnvVar) error { - if s == nil { +func (optSet *OptionSet) ParseEnv(vs []EnvVar) error { + if optSet == nil { return nil } @@ -249,7 +249,7 @@ func (s *OptionSet) ParseEnv(vs []EnvVar) error { envs[v.Name] = v.Value } - for i, opt := range *s { + for i, opt := range *optSet { if opt.Env == "" { continue } @@ -267,7 +267,7 @@ func (s *OptionSet) ParseEnv(vs []EnvVar) error { continue } - (*s)[i].ValueSource = ValueSourceEnv + (*optSet)[i].ValueSource = ValueSourceEnv if err := opt.Value.Set(envVal); err != nil { merr = multierror.Append( merr, xerrors.Errorf("parse %q: %w", opt.Name, err), @@ -280,14 +280,14 @@ func (s *OptionSet) ParseEnv(vs []EnvVar) error { // SetDefaults sets the default values for each Option, skipping values // that already have a value source. -func (s *OptionSet) SetDefaults() error { - if s == nil { +func (optSet *OptionSet) SetDefaults() error { + if optSet == nil { return nil } var merr *multierror.Error - for i, opt := range *s { + for i, opt := range *optSet { // Skip values that may have already been set by the user. if opt.ValueSource != ValueSourceNone { continue @@ -307,7 +307,7 @@ func (s *OptionSet) SetDefaults() error { ) continue } - (*s)[i].ValueSource = ValueSourceDefault + (*optSet)[i].ValueSource = ValueSourceDefault if err := opt.Value.Set(opt.Default); err != nil { merr = multierror.Append( merr, xerrors.Errorf("parse %q: %w", opt.Name, err), @@ -319,9 +319,9 @@ func (s *OptionSet) SetDefaults() error { // ByName returns the Option with the given name, or nil if no such option // exists. -func (s *OptionSet) ByName(name string) *Option { - for i := range *s { - opt := &(*s)[i] +func (optSet *OptionSet) ByName(name string) *Option { + for i := range *optSet { + opt := &(*optSet)[i] if opt.Name == name { return opt } From 9e68d53f2bafc9b0286117a3cef961b7920e6fa4 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 29 Sep 2023 08:51:09 -0500 Subject: [PATCH 11/20] fixup! rename optionset receiver --- cli/clibase/yaml.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/cli/clibase/yaml.go b/cli/clibase/yaml.go index fbb1529b3bf4d..9bb1763571eb4 100644 --- a/cli/clibase/yaml.go +++ b/cli/clibase/yaml.go @@ -51,12 +51,12 @@ func deepMapNode(n *yaml.Node, path []string, headComment string) *yaml.Node { // the stack. // // It is isomorphic with FromYAML. -func (s *OptionSet) MarshalYAML() (any, error) { +func (optSet *OptionSet) MarshalYAML() (any, error) { root := yaml.Node{ Kind: yaml.MappingNode, } - for _, opt := range *s { + for _, opt := range *optSet { if opt.YAML == "" { continue } @@ -222,7 +222,7 @@ 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 { +func (optSet *OptionSet) UnmarshalYAML(rootNode *yaml.Node) error { // 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 { @@ -240,8 +240,8 @@ func (s *OptionSet) UnmarshalYAML(rootNode *yaml.Node) error { matchedNodes := make(map[string]*yaml.Node, len(yamlNodes)) var merr error - for i := range *s { - opt := &(*s)[i] + for i := range *optSet { + opt := &(*optSet)[i] if opt.YAML == "" { continue } From 27cc9f3ac59f91db7afda33fcae3a59430d3e83f Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 29 Sep 2023 08:51:32 -0500 Subject: [PATCH 12/20] Ignore unused arg --- cli/clibase/values.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/clibase/values.go b/cli/clibase/values.go index f4d7e523986b1..3a7b2c3919a5a 100644 --- a/cli/clibase/values.go +++ b/cli/clibase/values.go @@ -430,7 +430,7 @@ func (discardValue) Type() string { return "discard" } -func (discardValue) UnmarshalJSON(data []byte) error { +func (discardValue) UnmarshalJSON([]byte) error { return nil } From a5029d1a50916b8c4d5729b226b295066952b488 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 29 Sep 2023 10:05:47 -0500 Subject: [PATCH 13/20] Add json marshal/unmarshal test --- cli/clibase/option.go | 6 +- cli/clibase/option_test.go | 131 +++++++++++++++++++++++++++++++++++++ cli/clibase/values.go | 20 ++++++ 3 files changed, 155 insertions(+), 2 deletions(-) diff --git a/cli/clibase/option.go b/cli/clibase/option.go index 2b84140ffdb8a..d881ff6e00c9f 100644 --- a/cli/clibase/option.go +++ b/cli/clibase/option.go @@ -146,8 +146,10 @@ OptionSetDecodeLoop: // Set the opt's value opt.Value = (*optSet)[i].Value } else { - // Discard the value if it's nil. - opt.Value = DiscardValue + // Hopefully the user passed empty values in the option set. There is no easy way + // to tell, and if we do not do this, it breaks json.Marshal if we do it again on + // this new option set. + opt.Value = (*optSet)[i].Value } // Override the existing. (*optSet)[i] = opt diff --git a/cli/clibase/option_test.go b/cli/clibase/option_test.go index f1b881d94408e..0164ac50818d7 100644 --- a/cli/clibase/option_test.go +++ b/cli/clibase/option_test.go @@ -1,11 +1,16 @@ package clibase_test import ( + "bytes" + "encoding/json" + "regexp" "testing" "github.com/stretchr/testify/require" "github.com/coder/coder/v2/cli/clibase" + "github.com/coder/coder/v2/coderd/coderdtest" + "github.com/coder/coder/v2/codersdk" ) func TestOptionSet_ParseFlags(t *testing.T) { @@ -201,3 +206,129 @@ func TestOptionSet_ParseEnv(t *testing.T) { require.EqualValues(t, expected, actual.Value) }) } + +func TestOptionSet_JsonMarshal(t *testing.T) { + t.Parallel() + + t.Run("RegexCase", func(t *testing.T) { + val := clibase.Regexp(*regexp.MustCompile(".*")) + opts := clibase.OptionSet{ + clibase.Option{ + Name: "Regex", + Value: &val, + Default: ".*", + }, + } + data, err := json.Marshal(opts) + require.NoError(t, err, "marshal option set") + + var foundVal clibase.Regexp + newOpts := clibase.OptionSet{ + clibase.Option{ + Name: "Regex", + Value: &foundVal, + }, + } + err = json.Unmarshal(data, &newOpts) + require.NoError(t, err, "unmarshal option set") + + require.EqualValues(t, opts[0].Value.String(), newOpts[0].Value.String()) + }) + + t.Run("AllValues", func(t *testing.T) { + t.Parallel() + + vals := coderdtest.DeploymentValues(t) + opts := vals.Options() + sources := []clibase.ValueSource{ + clibase.ValueSourceNone, + clibase.ValueSourceFlag, + clibase.ValueSourceEnv, + clibase.ValueSourceYAML, + clibase.ValueSourceDefault, + } + for i := range opts { + opts[i].ValueSource = sources[i%len(sources)] + } + + data, err := json.Marshal(opts) + require.NoError(t, err, "marshal option set") + + newOpts := (&codersdk.DeploymentValues{}).Options() + err = json.Unmarshal(data, &newOpts) + require.NoError(t, err, "unmarshal option set") + + for i := range opts { + exp := opts[i] + found := newOpts[i] + + compareOptions(t, exp, found) + } + + thirdOpts := (&codersdk.DeploymentValues{}).Options() + data, err = json.Marshal(newOpts) + require.NoError(t, err, "marshal option set") + + err = json.Unmarshal(data, &thirdOpts) + require.NoError(t, err, "unmarshal option set") + // Compare to the original opts again + for i := range opts { + exp := opts[i] + found := thirdOpts[i] + + compareOptions(t, exp, found) + } + }) +} + +func compareOptions(t *testing.T, exp, found clibase.Option) { + require.Equalf(t, exp.Name, found.Name, "option name %q", exp.Name) + require.Equalf(t, exp.Description, found.Description, "option description %q", exp.Name) + require.Equalf(t, exp.Required, found.Required, "option required %q", exp.Name) + require.Equalf(t, exp.Flag, found.Flag, "option flag %q", exp.Name) + require.Equalf(t, exp.FlagShorthand, found.FlagShorthand, "option flag shorthand %q", exp.Name) + require.Equalf(t, exp.Env, found.Env, "option env %q", exp.Name) + require.Equalf(t, exp.YAML, found.YAML, "option yaml %q", exp.Name) + require.Equalf(t, exp.Default, found.Default, "option default %q", exp.Name) + require.Equalf(t, exp.ValueSource, found.ValueSource, "option value source %q", exp.Name) + require.Equalf(t, exp.Hidden, found.Hidden, "option hidden %q", exp.Name) + require.Equalf(t, exp.Annotations, found.Annotations, "option annotations %q", exp.Name) + require.Equalf(t, exp.Group, found.Group, "option group %q", exp.Name) + // UseInstead is the same comparison problem, just check the length + require.Equalf(t, len(exp.UseInstead), len(found.UseInstead), "option use instead %q", exp.Name) + + compareValues(t, exp, found) +} + +func compareValues(t *testing.T, exp, found clibase.Option) { + if exp.Value.String() != found.Value.String() && found.Value.String() == "" { + // If the string values are different, this can be a "nil" issue. + // So only run this case if the found string is the empty string. + // We use MarshalYAML for struct strings, and it will return an + // empty string '""' for nil slices/maps/etc. + // So use json to compare. + + expJson, err := json.Marshal(exp.Value) + require.NoError(t, err, "marshal") + foundJson, err := json.Marshal(found.Value) + require.NoError(t, err, "marshal") + + expJson = normalizeJSON(expJson) + foundJson = normalizeJSON(foundJson) + require.Equalf(t, string(expJson), string(foundJson), "option value %q", exp.Name) + } else { + require.Equal(t, + exp.Value.String(), + found.Value.String(), + "option value %q", exp.Name) + } +} + +// normalizeJSON handles the fact that an empty map/slice is not the same +// as a nil empty/slice. For our purposes, they are the same. +func normalizeJSON(data []byte) []byte { + if bytes.Compare(data, []byte("[]")) == 0 || bytes.Compare(data, []byte("{}")) == 0 { + return []byte("null") + } + return data +} diff --git a/cli/clibase/values.go b/cli/clibase/values.go index 3a7b2c3919a5a..19179c8eae34b 100644 --- a/cli/clibase/values.go +++ b/cli/clibase/values.go @@ -381,6 +381,7 @@ func (s *Struct[T]) String() string { func (s *Struct[T]) MarshalYAML() (interface{}, error) { var n yaml.Node err := n.Encode(s.Value) + if err != nil { return nil, err } @@ -493,6 +494,25 @@ func (e *Enum) String() string { type Regexp regexp.Regexp +func (r *Regexp) MarshalJSON() ([]byte, error) { + return json.Marshal(r.String()) +} + +func (r *Regexp) UnmarshalJSON(data []byte) error { + var source string + err := json.Unmarshal(data, &source) + if err != nil { + return err + } + + exp, err := regexp.Compile(source) + if err != nil { + return xerrors.Errorf("invalid regex expression: %w", err) + } + *r = Regexp(*exp) + return nil +} + func (r *Regexp) MarshalYAML() (interface{}, error) { return yaml.Node{ Kind: yaml.ScalarNode, From ab2ecd955fa5ecc00b52e4a70ea12fcb5a124d81 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 29 Sep 2023 10:08:39 -0500 Subject: [PATCH 14/20] Fixups --- cli/clibase/option.go | 13 +++++++------ coderd/coderd.go | 2 +- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/cli/clibase/option.go b/cli/clibase/option.go index d881ff6e00c9f..74eedf5e5ed8f 100644 --- a/cli/clibase/option.go +++ b/cli/clibase/option.go @@ -107,7 +107,8 @@ type OptionSet []Option // // The value is discarded if it's type cannot be inferred. This behavior just // feels "safer", although it should never happen if the correct option set -// is passed in. +// is passed in. The situation where this could occur is if a client and server +// are on different versions with different options. func (optSet *OptionSet) UnmarshalJSON(data []byte) error { dec := json.NewDecoder(bytes.NewBuffer(data)) // Should be a json array, so consume the starting open bracket. @@ -136,23 +137,23 @@ OptionSetDecodeLoop: // Try to see if the option already exists in the option set. // If it does, just update the existing option. - for i, have := range *optSet { + for optIndex, have := range *optSet { if have.Name == opt.Name { if jValue != nil { - err := json.Unmarshal(jValue, &(*optSet)[i].Value) + err := json.Unmarshal(jValue, &(*optSet)[optIndex].Value) if err != nil { return xerrors.Errorf("decode option %q value: %w", have.Name, err) } // Set the opt's value - opt.Value = (*optSet)[i].Value + opt.Value = (*optSet)[optIndex].Value } else { // Hopefully the user passed empty values in the option set. There is no easy way // to tell, and if we do not do this, it breaks json.Marshal if we do it again on // this new option set. - opt.Value = (*optSet)[i].Value + opt.Value = (*optSet)[optIndex].Value } // Override the existing. - (*optSet)[i] = opt + (*optSet)[optIndex] = opt // Go to the next option to decode. continue OptionSetDecodeLoop } diff --git a/coderd/coderd.go b/coderd/coderd.go index 24dd9c7bfe540..097ed1e6ff6ba 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -153,7 +153,7 @@ type Options struct { MetricsCacheRefreshInterval time.Duration AgentStatsRefreshInterval time.Duration DeploymentValues *codersdk.DeploymentValues - // DeploymentOptions do contain the copy of DeploymentValues, but contain + // DeploymentOptions do contain the copy of DeploymentValues, and contain // contextual information about how the values were set. // Do not use DeploymentOptions to retrieve values, use DeploymentValues instead. // All secrets values are stripped. From 1afa4fe47e43394a19d568e7a99088089bfe2320 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 29 Sep 2023 10:57:06 -0500 Subject: [PATCH 15/20] Use nil for secret types --- cli/clibase/option_test.go | 13 ++++++++++--- codersdk/deployment.go | 9 +-------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/cli/clibase/option_test.go b/cli/clibase/option_test.go index 0164ac50818d7..1cc7a873c7337 100644 --- a/cli/clibase/option_test.go +++ b/cli/clibase/option_test.go @@ -6,6 +6,7 @@ import ( "regexp" "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/coder/coder/v2/cli/clibase" @@ -211,6 +212,8 @@ func TestOptionSet_JsonMarshal(t *testing.T) { t.Parallel() t.Run("RegexCase", func(t *testing.T) { + t.Parallel() + val := clibase.Regexp(*regexp.MustCompile(".*")) opts := clibase.OptionSet{ clibase.Option{ @@ -282,6 +285,8 @@ func TestOptionSet_JsonMarshal(t *testing.T) { } func compareOptions(t *testing.T, exp, found clibase.Option) { + t.Helper() + require.Equalf(t, exp.Name, found.Name, "option name %q", exp.Name) require.Equalf(t, exp.Description, found.Description, "option description %q", exp.Name) require.Equalf(t, exp.Required, found.Required, "option required %q", exp.Name) @@ -301,7 +306,9 @@ func compareOptions(t *testing.T, exp, found clibase.Option) { } func compareValues(t *testing.T, exp, found clibase.Option) { - if exp.Value.String() != found.Value.String() && found.Value.String() == "" { + t.Helper() + + if (exp.Value == nil || found.Value == nil) || (exp.Value.String() != found.Value.String() && found.Value.String() == "") { // If the string values are different, this can be a "nil" issue. // So only run this case if the found string is the empty string. // We use MarshalYAML for struct strings, and it will return an @@ -315,9 +322,9 @@ func compareValues(t *testing.T, exp, found clibase.Option) { expJson = normalizeJSON(expJson) foundJson = normalizeJSON(foundJson) - require.Equalf(t, string(expJson), string(foundJson), "option value %q", exp.Name) + assert.Equalf(t, string(expJson), string(foundJson), "option value %q", exp.Name) } else { - require.Equal(t, + assert.Equal(t, exp.Value.String(), found.Value.String(), "option value %q", exp.Name) diff --git a/codersdk/deployment.go b/codersdk/deployment.go index 3caf6e2fc9881..be629236d344b 100644 --- a/codersdk/deployment.go +++ b/codersdk/deployment.go @@ -1768,14 +1768,7 @@ func DeploymentOptionsWithoutSecrets(set clibase.OptionSet) clibase.OptionSet { for _, opt := range set { cpyOpt := opt if IsSecretDeploymentOption(cpyOpt) { - switch cpyOpt.Value.Type() { - case "string-array": - var empty clibase.StringArray - cpyOpt.Value = &empty - default: - var empty clibase.String - cpyOpt.Value = &empty - } + cpyOpt.Value = nil } cpy = append(cpy, cpyOpt) } From 03a78834e463eb7e49213cbaf177bb5f001fed32 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 29 Sep 2023 11:09:27 -0500 Subject: [PATCH 16/20] Add unit test for missing opts case --- cli/clibase/option_test.go | 40 +++++++++++++++++++++++++++++++++----- 1 file changed, 35 insertions(+), 5 deletions(-) diff --git a/cli/clibase/option_test.go b/cli/clibase/option_test.go index 1cc7a873c7337..e4d4b7c7f1e04 100644 --- a/cli/clibase/option_test.go +++ b/cli/clibase/option_test.go @@ -211,6 +211,36 @@ func TestOptionSet_ParseEnv(t *testing.T) { func TestOptionSet_JsonMarshal(t *testing.T) { t.Parallel() + // This unit test ensures if the source optionset is missing the option + // and cannot determine the type, it will not panic. The unmarshal will + // succeed with a best effort. + t.Run("MissingSrcOption", func(t *testing.T) { + t.Parallel() + + var str clibase.String = "something" + var arr clibase.StringArray = []string{"foo", "bar"} + opts := clibase.OptionSet{ + clibase.Option{ + Name: "StringOpt", + Value: &str, + }, + clibase.Option{ + Name: "ArrayOpt", + Value: &arr, + }, + } + data, err := json.Marshal(opts) + require.NoError(t, err, "marshal option set") + + tgt := clibase.OptionSet{} + err = json.Unmarshal(data, &tgt) + require.NoError(t, err, "unmarshal option set") + for i := range opts { + compareOptionsExceptValues(t, opts[i], tgt[i]) + require.Empty(t, tgt[i].Value.String(), "unknown value types are empty") + } + }) + t.Run("RegexCase", func(t *testing.T) { t.Parallel() @@ -265,7 +295,8 @@ func TestOptionSet_JsonMarshal(t *testing.T) { exp := opts[i] found := newOpts[i] - compareOptions(t, exp, found) + compareOptionsExceptValues(t, exp, found) + compareValues(t, exp, found) } thirdOpts := (&codersdk.DeploymentValues{}).Options() @@ -279,12 +310,13 @@ func TestOptionSet_JsonMarshal(t *testing.T) { exp := opts[i] found := thirdOpts[i] - compareOptions(t, exp, found) + compareOptionsExceptValues(t, exp, found) + compareValues(t, exp, found) } }) } -func compareOptions(t *testing.T, exp, found clibase.Option) { +func compareOptionsExceptValues(t *testing.T, exp, found clibase.Option) { t.Helper() require.Equalf(t, exp.Name, found.Name, "option name %q", exp.Name) @@ -301,8 +333,6 @@ func compareOptions(t *testing.T, exp, found clibase.Option) { require.Equalf(t, exp.Group, found.Group, "option group %q", exp.Name) // UseInstead is the same comparison problem, just check the length require.Equalf(t, len(exp.UseInstead), len(found.UseInstead), "option use instead %q", exp.Name) - - compareValues(t, exp, found) } func compareValues(t *testing.T, exp, found clibase.Option) { From 93aa42689a4b1dad7feb3f389b48ce856dcc74b1 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 29 Sep 2023 11:10:49 -0500 Subject: [PATCH 17/20] Linting --- cli/clibase/option_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/cli/clibase/option_test.go b/cli/clibase/option_test.go index e4d4b7c7f1e04..79db040ea7e47 100644 --- a/cli/clibase/option_test.go +++ b/cli/clibase/option_test.go @@ -345,14 +345,14 @@ func compareValues(t *testing.T, exp, found clibase.Option) { // empty string '""' for nil slices/maps/etc. // So use json to compare. - expJson, err := json.Marshal(exp.Value) + expJSON, err := json.Marshal(exp.Value) require.NoError(t, err, "marshal") - foundJson, err := json.Marshal(found.Value) + foundJSON, err := json.Marshal(found.Value) require.NoError(t, err, "marshal") - expJson = normalizeJSON(expJson) - foundJson = normalizeJSON(foundJson) - assert.Equalf(t, string(expJson), string(foundJson), "option value %q", exp.Name) + expJSON = normalizeJSON(expJSON) + foundJSON = normalizeJSON(foundJSON) + assert.Equalf(t, string(expJSON), string(foundJSON), "option value %q", exp.Name) } else { assert.Equal(t, exp.Value.String(), @@ -364,7 +364,7 @@ func compareValues(t *testing.T, exp, found clibase.Option) { // normalizeJSON handles the fact that an empty map/slice is not the same // as a nil empty/slice. For our purposes, they are the same. func normalizeJSON(data []byte) []byte { - if bytes.Compare(data, []byte("[]")) == 0 || bytes.Compare(data, []byte("{}")) == 0 { + if bytes.Equal(data, []byte("[]")) || bytes.Equal(data, []byte("{}")) { return []byte("null") } return data From 42fb5d012ed9408eb9d28767329a585d47dc573a Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 29 Sep 2023 11:16:32 -0500 Subject: [PATCH 18/20] Fix unit test --- cli/server_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/cli/server_test.go b/cli/server_test.go index ccb5730e52ce0..dbbff56d9ac3d 100644 --- a/cli/server_test.go +++ b/cli/server_test.go @@ -1504,6 +1504,9 @@ func TestServer(t *testing.T) { gotConfig.Options.ByName("Config Path").Value.Set("") // We check the options individually for better error messages. for i := range wantConfig.Options { + // ValueSource is not going to be correct on the `want`, so just + // match that field. + wantConfig.Options[i].ValueSource = gotConfig.Options[i].ValueSource assert.Equal( t, wantConfig.Options[i], gotConfig.Options[i], From cbb69bcee70f2f4b4a95b762d598efb96371ddd2 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 29 Sep 2023 11:17:13 -0500 Subject: [PATCH 19/20] Formatting --- cli/clibase/values.go | 1 - 1 file changed, 1 deletion(-) diff --git a/cli/clibase/values.go b/cli/clibase/values.go index 19179c8eae34b..d390fe2f89bc6 100644 --- a/cli/clibase/values.go +++ b/cli/clibase/values.go @@ -381,7 +381,6 @@ func (s *Struct[T]) String() string { func (s *Struct[T]) MarshalYAML() (interface{}, error) { var n yaml.Node err := n.Encode(s.Value) - if err != nil { return nil, err } From b5a9164d7485894c9ad327c20f88c02e88070ce3 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 29 Sep 2023 11:25:44 -0500 Subject: [PATCH 20/20] Increment the error context counter --- cli/clibase/option.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/cli/clibase/option.go b/cli/clibase/option.go index 74eedf5e5ed8f..b9a2e871d05df 100644 --- a/cli/clibase/option.go +++ b/cli/clibase/option.go @@ -134,6 +134,11 @@ OptionSetDecodeLoop: if err != nil { return xerrors.Errorf("decode %d option: %w", i, err) } + // This counter is used to contextualize errors to show which element of + // the array we failed to decode. It is only used in the error above, as + // if the above works, we can instead use the Option.Name which is more + // descriptive and useful. So increment here for the next decode. + i++ // Try to see if the option already exists in the option set. // If it does, just update the existing option. @@ -163,7 +168,6 @@ OptionSetDecodeLoop: // We do this because we cannot infer the type of the value. opt.Value = DiscardValue *optSet = append(*optSet, opt) - i++ } t, err = dec.Token()