Skip to content
This repository has been archived by the owner on Mar 16, 2024. It is now read-only.

Refactor GenericMap to prevent managedField issues #2080

Merged
merged 2 commits into from Sep 9, 2023

Conversation

njhale
Copy link
Contributor

@njhale njhale commented Aug 16, 2023

This change lets us generate the correct OpenAPI schema for GenericMap without the need to mutate the schema for fields, by name, at runtime. It will stop the "managedFields" message from being printed for preexisting usages of GenericMap and prevent it for all new usages going forward.

@njhale njhale force-pushed the refactor/gm branch 5 times, most recently from 7de55c1 to 4f584ea Compare August 22, 2023 19:32
@njhale njhale marked this pull request as ready for review August 22, 2023 19:33
@njhale njhale force-pushed the refactor/gm branch 3 times, most recently from d7718d0 to dd3874a Compare August 22, 2023 20:08
@njhale
Copy link
Contributor Author

njhale commented Aug 22, 2023

Looks like I'm still having some issues here. Will update.

@njhale njhale force-pushed the refactor/gm branch 4 times, most recently from 9a8f556 to 4f2c319 Compare August 25, 2023 15:54
@njhale
Copy link
Contributor Author

njhale commented Aug 25, 2023

OK -- this is all ready to go

@njhale njhale force-pushed the refactor/gm branch 3 times, most recently from f8b8850 to 46804c1 Compare August 25, 2023 21:38
@@ -749,7 +749,7 @@ type Secret struct {
Name string `json:"name,omitempty"`
Description string `json:"description,omitempty"`
Type string `json:"type,omitempty"`
Params GenericMap `json:"params,omitempty"`
Params *GenericMap `json:"params,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Since this is a pointer everywhere I feel like the chance of panic is extremely high. Like I'm reading all the changes and trying to see if you first checked for nil. What if you add a GetData() to GenericMap that will first check if the receiver is nil and then return nil. So then existing code that did foo.DeployArgs and just be changed to foo.DeployArgs.GetData() and still be safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like this. Added.

@njhale njhale force-pushed the refactor/gm branch 7 times, most recently from f0d9ab2 to ab06f9f Compare August 28, 2023 09:59
pkg/apis/internal.acorn.io/v1/map.go Outdated Show resolved Hide resolved
pkg/apis/internal.acorn.io/v1/map.go Outdated Show resolved Hide resolved
pkg/apis/internal.acorn.io/v1/map.go Outdated Show resolved Hide resolved
pkg/apis/internal.acorn.io/v1/map.go Outdated Show resolved Hide resolved
pkg/appdefinition/params_test.go Outdated Show resolved Hide resolved
@njhale
Copy link
Contributor Author

njhale commented Sep 5, 2023

The integration tests are failing in a way that makes it hard to tell what the exact problem is. TestServiceInfo fails on line 30 with:

 #5 DONE 0.0s
time="2023-09-04T03:16:12Z" level=error msg="apiserver received an error that is not an metav1.Status: &errors.errorString{s:\"failed to find image information for nested acorn [nested]\"}: failed to find image information for nested acorn [nested]\n" error="<nil>"
    services_test.go:30: App.api.acorn.io "divine-moon" is invalid: spec.image: Invalid value: "2eb7bb425cf1072e11a12b3a822d29ae019f38994a15e8be54b2b8bea66eb394": failed to find image information for nested acorn [nested]

when attempting to run an Acorn built earlier in the same test

I know that it must be happening client-side since I'm executing the test in my branch against a local instance running runtime:main. Unless I'm missing something obvious, I suspect there's some issue with marshaling and/or marshaling GenericMap that silently breaks the build|run. I haven't managed to pin down exactly where this is happening yet though.

@njhale njhale force-pushed the refactor/gm branch 2 times, most recently from 167448a to e0f81b1 Compare September 5, 2023 17:46
@njhale
Copy link
Contributor Author

njhale commented Sep 6, 2023

This is all ready for review. Thanks for the patience folks.

@@ -140,7 +140,7 @@ func ToAppUpdate(ctx context.Context, c Client, name string, opts *AppUpdateOpti
app.Spec.Environment = mergeEnv(app.Spec.Environment, opts.Env)
app.Spec.Labels = mergeLabels(app.Spec.Labels, opts.Labels)
app.Spec.Annotations = mergeLabels(app.Spec.Annotations, opts.Annotations)
app.Spec.DeployArgs = typed.Concat(app.Spec.DeployArgs, opts.DeployArgs)
app.Spec.DeployArgs = app.Spec.DeployArgs.Merge(v1.NewGenericMap(opts.DeployArgs))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really like the way this looks. Any suggestions?

pkg/appdefinition/params_test.go Outdated Show resolved Hide resolved
return a.OriginalImage == b.OriginalImage &&
a.Context == b.Context &&
a.Acornfile == b.Acornfile &&
equality.Semantic.DeepEqual(a.BuildArgs.GetData(), b.BuildArgs.GetData())
Copy link
Contributor Author

@njhale njhale Sep 6, 2023

Choose a reason for hiding this comment

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

This was added to satisfy a preexisting requirement in the code that depends on equality between the zero value of v1.GenericMap and nil; when we switched from a named map type to a struct, the default logic provided by equality.Semantic was no longer sufficient.

Copy link
Contributor

@tylerslaton tylerslaton left a comment

Choose a reason for hiding this comment

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

Have you tried integrating this into manager and seeing what happens?

@njhale
Copy link
Contributor Author

njhale commented Sep 6, 2023

Have you tried integrating this into manager and seeing what happens?

I've smoke-tested the backwards and forwards compatibility in runtime, so this should work just fine. A PR for manager using this branch is on its way.

Switch GenericMap from a named `map[string]interface{}` type
to a struct with a Data field. This allows us to override the openapi
spec once, at the type level, instead of retroactively at runtime for
each new field of type GenericMap added to our API structs.

This prevents the "managedFields" log that occurs when we forget to add
new fields to the OpenAPI modification logic.

Signed-off-by: Nick Hale <4175918+njhale@users.noreply.github.com>
Signed-off-by: Nick Hale <4175918+njhale@users.noreply.github.com>
@njhale njhale merged commit f61acb7 into acorn-io:main Sep 9, 2023
4 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants