Skip to content

chore: color value_source for deployment values #9922

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 21 commits into from
Sep 29, 2023
Merged

chore: color value_source for deployment values #9922

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
ac997a9
chore: Return populated options vs a blank
Emyrk Sep 28, 2023
3748430
Strip secret values
Emyrk Sep 28, 2023
07588ec
Merge remote-tracking branch 'origin/main' into stevenmasley/deployme…
Emyrk Sep 28, 2023
e1a8997
Color the value source for a deployment value
Emyrk Sep 28, 2023
a6cc10e
Use theme colors
Emyrk Sep 28, 2023
be1aec3
try going with a blue
Emyrk Sep 28, 2023
dc67bb7
Fix json marshal of secrets
Emyrk Sep 28, 2023
53b147b
Fix unmarshal json for optionsets
Emyrk Sep 29, 2023
b1058a6
Add comment
Emyrk Sep 29, 2023
6ce3af7
Add diff color to config option when it is source
BrunoQuaresma Sep 29, 2023
5049884
rename optionset receiver
Emyrk Sep 29, 2023
9e68d53
fixup! rename optionset receiver
Emyrk Sep 29, 2023
27cc9f3
Ignore unused arg
Emyrk Sep 29, 2023
a5029d1
Add json marshal/unmarshal test
Emyrk Sep 29, 2023
ab2ecd9
Fixups
Emyrk Sep 29, 2023
1afa4fe
Use nil for secret types
Emyrk Sep 29, 2023
03a7883
Add unit test for missing opts case
Emyrk Sep 29, 2023
93aa426
Linting
Emyrk Sep 29, 2023
42fb5d0
Fix unit test
Emyrk Sep 29, 2023
cbb69bc
Formatting
Emyrk Sep 29, 2023
b5a9164
Increment the error context counter
Emyrk Sep 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Fix json marshal of secrets
  • Loading branch information
@Emyrk
Emyrk committed Sep 28, 2023
commit dc67bb7f4ff3f66458fc9410630f4991d0adec64
49 changes: 24 additions & 25 deletions coderd/coderdtest/coderdtest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
10 changes: 8 additions & 2 deletions codersdk/deployment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Oooh this is where I broke the json.Unmarshal(). @johnstcn @mafredri

So json.Unmarshal does unmarshal in place for arrays, so all the types were working correctly. When I was zeroing the secrets, I figured a string is fine, but it is not. I should have used a nil.

So this made a clibase.String try to unmarshal into whatever the secret was, which did not work. If I instead just use a nil, then it all works 🤦‍♂️.

So I can delete all the json unmarshal stuff I wrote, but my json unmarshal handles all the edge cases that would usually just fail (see https://goplay.tools/snippet/GPxfjtnfvys).

So I can delete my unmarshal code, and if your client and version have different option sets, the call will fail. 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

so if you have client version X, and we add an option in server version Y, then the unmarshal will fail when client tries to get DeploymentOptions?

Yes.

Copy link
Member

Choose a reason for hiding this comment

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

I'd rather have a custom UnmarshalJSON method than a potential regression between versions :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we should keep this complicated unmarshal logic. It is more complicated, but with testing, it is safe and only affects the codersdk.Client. If we do not have this code, then we can unintentionally just break the client, and there is nothing the client can do to resolve this since the JSON from the server is the issue at hand.

}
cpy = append(cpy, cpyOpt)
}
Expand Down