chore: color `value_source` for deployment values #9922

Emyrk · 2023-09-28T21:25:08Z

Implementation of Optionset Unmarshal

Before this change, all Value fields of Options were nil and so unmarshalling was ok. Now that I am passing a value, the UnmarshalJSON function gets complicated because we cannot infer types easily.

I got this to work, although we don't use these values anywhere in the Golang, so it didn't really matter on the golang side. On the Typescript side, we just use the any type and type case to the types we know they are. 🤷‍♂️

It's a little janky. To move to a more statically typed struct, we would need to make OptionSet a struct with fields, rather than a slice? Or extend the pflag.Value interface to include some Type() fields we can do some type interferences on. Or just make an any type that implements the interface??

For now, I think this solution is pretty good.

What this does

Indicates where the value is set. Should we add a tooltip or something? Or is this intuitive enough?

How it looks in green

![Screenshot from 2023-09-28 16-29-30](https://github.com/coder/coder/assets/5446298/bfe3ac8a-b2ff-470c-9939-df17307c6c64)

…nt_opts_source

Emyrk · 2023-09-29T03:53:10Z

cli/clibase/option.go

+	// 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
+	}


Since the values are behind an interface, if we do not know how to implement the .String() method, then having the value in an unknown form is pointless.

We could add some logic to try and detect primitive options, but I do not see the value. We should always have the full set of Options when we unmarshal, so we should always get the correct types and values.

johnstcn · 2023-09-29T08:16:25Z

coderd/coderd.go

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


Why not just strip all values from DeploymentOptions then? Too many allocations?

I wanted to go this route, but the UI grabs the value from the option. Values are returned in the parsed form, and the option form:

coder/coderd/deployment.go

Lines 34 to 37 in b1058a6

codersdk.DeploymentConfig{

Values: values,

Options: api.DeploymentOptions,

},

There is no way to associate the values between the two, so the value needs to be present on the Option for the UI to know.

I feel like this could use a bit of a refactor, but it is working and not really that important.

codersdk/deployment.go

BrunoQuaresma

FE looks good.

cli/clibase/option.go

mafredri

Neat feature! While I understood the shortcoming, I didn't quite grasp why we had to work around it in UnmarhsalJSON like we did. But I'm fine with it if you think that's the best approach. It would be ideal if we could just change our structures so that there's no need for it, though.

coderd/coderd.go

cli/clibase/option.go

mafredri · 2023-09-29T14:45:24Z

cli/clibase/option.go

+//
+// 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.


I'm not sure under what circumstances this Unmarshal is run? Is there a chance of conflicts between server versions (API response) and CLI version (decode)?

Yup exactly. The new version can have more deployment options, so the old version has nothing to match it against.

Emyrk · 2023-09-29T15:29:00Z

Neat feature! While I understood the shortcoming, I didn't quite grasp why we had to work around it in UnmarshalJSON like we did. But I'm fine with it if you think that's the best approach. It would be ideal if we could just change our structures so that there's no need for it, though.

Yea this was an annoying headache. Ideally we change the the data structure, but that would require us to change the slice into a struct, because you cannot have typed slices where each element is different.

That change ripples aggressively through a lot of code on the FE and BE, and would be a much bigger change.

I am taking a step back and trying to understand how main is working... Because it must be working to some extent with the values, but I kept running into this unmarshal error demonstrated here:

https://goplay.tools/snippet/F5I4fFRB3au

Essentially when you call UnmarshalJSON on an interface with methods, the unmarshal target needs to implement those methods. So if you do not have the underlying type ready, then the unmarshal will not work. We always do (&codersdk.DeploymentValues{}).Options() before we call Unmarshal, but that would be the slice is being unmarshaled in place? Like the elements are being unmarshaled into each index?

If main is already doing that, maybe there is something about json.Unmarshal I did not know about.

Emyrk · 2023-09-29T15:52:23Z

codersdk/deployment.go

+			case "string-array":
+				var empty clibase.StringArray
+				cpyOpt.Value = &empty
+			default:
+				var empty clibase.String
+				cpyOpt.Value = &empty
+			}


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

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.

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

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.

cli/clibase/option.go

Emyrk added 4 commits September 28, 2023 15:49

chore: Return populated options vs a blank

ac997a9

Strip secret values

3748430

Merge remote-tracking branch 'origin/main' into stevenmasley/deployme…

07588ec

…nt_opts_source

Color the value source for a deployment value

e1a8997

github-actions bot assigned Emyrk Sep 28, 2023

Emyrk changed the title ~~chore: Color value_source for deployment values~~ chore: color value_source for deployment values Sep 28, 2023

Use theme colors

a6cc10e

Emyrk requested review from mafredri and BrunoQuaresma September 28, 2023 21:34

Emyrk added 2 commits September 28, 2023 16:37

try going with a blue

be1aec3

Fix json marshal of secrets

dc67bb7

Emyrk removed the request for review from mafredri September 28, 2023 21:57

Fix unmarshal json for optionsets

53b147b

Emyrk commented Sep 29, 2023

View reviewed changes

Add comment

b1058a6

Emyrk requested a review from johnstcn September 29, 2023 03:55

johnstcn reviewed Sep 29, 2023

View reviewed changes

Add diff color to config option when it is source

6ce3af7

BrunoQuaresma approved these changes Sep 29, 2023

View reviewed changes

johnstcn reviewed Sep 29, 2023

View reviewed changes

cli/clibase/option.go Outdated Show resolved Hide resolved

Emyrk added 3 commits September 29, 2023 08:49

rename optionset receiver

5049884

fixup! rename optionset receiver

9e68d53

Ignore unused arg

27cc9f3

mafredri reviewed Sep 29, 2023

View reviewed changes

Emyrk added 2 commits September 29, 2023 10:05

Add json marshal/unmarshal test

a5029d1

Fixups

ab2ecd9

Emyrk commented Sep 29, 2023

View reviewed changes

Emyrk added 2 commits September 29, 2023 10:57

Use nil for secret types

1afa4fe

Add unit test for missing opts case

03a7883

Emyrk added 3 commits September 29, 2023 11:10

Linting

93aa426

Fix unit test

42fb5d0

Formatting

cbb69bc

mafredri approved these changes Sep 29, 2023

View reviewed changes

cli/clibase/option.go Show resolved Hide resolved

Increment the error context counter

b5a9164

johnstcn approved these changes Sep 29, 2023

View reviewed changes

Emyrk merged commit 92308be into main Sep 29, 2023

Emyrk deleted the stevenmasley/deployment_opts_source branch September 29, 2023 17:04

github-actions bot locked and limited conversation to collaborators Sep 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: color `value_source` for deployment values #9922

chore: color `value_source` for deployment values #9922

Emyrk commented Sep 28, 2023 •

edited

Loading

Emyrk Sep 29, 2023

johnstcn Sep 29, 2023

Emyrk Sep 29, 2023

BrunoQuaresma left a comment

mafredri left a comment

mafredri Sep 29, 2023

Emyrk Sep 29, 2023

Emyrk commented Sep 29, 2023

Emyrk Sep 29, 2023

Emyrk Sep 29, 2023

johnstcn Sep 29, 2023

Emyrk Sep 29, 2023

	codersdk.DeploymentConfig{
	Values: values,
	Options: api.DeploymentOptions,
	},

chore: color value_source for deployment values #9922

chore: color value_source for deployment values #9922

Conversation

Emyrk commented Sep 28, 2023 • edited Loading

Implementation of Optionset Unmarshal

What this does

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrunoQuaresma left a comment

Choose a reason for hiding this comment

mafredri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Emyrk commented Sep 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chore: color `value_source` for deployment values #9922

chore: color `value_source` for deployment values #9922

Emyrk commented Sep 28, 2023 •

edited

Loading