Skip to content
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.

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

Improve spec.initProvider to spec.forProvider merge #240

Closed
Tracked by #4126
lsviben opened this issue Jul 27, 2023 · 0 comments · Fixed by #260
Closed
Tracked by #4126

Improve spec.initProvider to spec.forProvider merge #240

lsviben opened this issue Jul 27, 2023 · 0 comments · Fixed by #260
Assignees

Comments

@lsviben
Copy link
Contributor

lsviben commented Jul 27, 2023

Introduced with #237
When management policies are enabled, we are using the spec.initProvider field, and merging it to spec.forProvider. In addition,
we are calculating the fields which are part of spec.initProvider but not spec.forProvider and setting those fields into ignore_changes lifecycle hooks.

The issue here is that we are using mergo in a way that spec.initProvider fields override spec.forProvder fields:

// TODO(lsviben): Currently initProvider fields override forProvider
// fields if set. It should be the other way around.
// merge the initProvider and forProvider parameters
err = mergo.Merge(&params, initParams, mergo.WithSliceDeepCopy)
if err != nil {
	return nil, errors.Wrap(err, "cannot merge init and forProvider parameters")
}

This is due to using mergo.WithSliceDeepCopy which is on the other hand needed to merge arrays.

Ideally, spec.forProvider fields would take precedence over the spec.initProvider ones. Maybe there is an option with mergo to do something like that, or it wont be difficult to write ourselves.

But, as we don't recommenced or expect the same fields to be set, we could also error out if they set the same fields.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant