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

Function to convert Go struct back to config.Value #935

Merged
merged 7 commits into from
Nov 15, 2023
Merged

Conversation

pietern
Copy link
Contributor

@pietern pietern commented Oct 31, 2023

Changes

This PR is the counterpart to #904. With this change, we are able to convert a config.Value into a Go struct, make modifications to the Go struct, and reflect those changes in a new config.Value.

This functionality allows us to incrementally introduce this configuration representation to existing bundle mutators. Bundle mutators expect a *bundle.Bundle argument and mutate its configuration directly. These mutations are not reflected in the corresponding config.Value (once introduced), which means we cannot use the config.Value as source of truth until we update all mutators. To address this, we can run convert.ToTyped and convert.FromTyped at the mutator boundary (from bundle.Apply) and capture changes made to the Go struct. Then we can incrementally make mutators aware of the config.Value configuration and have them mutate that structure directly.

Tests

New unit tests pass.

Manual spot checks against the bundle configuration type.

@pietern pietern changed the title Function to convert Go struct back to config.Value Function to convert Go struct back to config.Value Oct 31, 2023
@pietern pietern requested a review from mgyucht November 1, 2023 13:57
Copy link
Contributor

@mgyucht mgyucht left a comment

Choose a reason for hiding this comment

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

Hm, I'm curious. If we can convert a go struct into config.Value, what would be the difference between this and Merge(ref, ConvertToConfigValue(src))? It might actually be about the same?

// Dereference pointer if necessary
for srcv.Kind() == reflect.Pointer {
if srcv.IsNil() {
return config.NilValue, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we return ref here instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No; if the Go structure has nilled a pointer that wasn't nil before, it means that chunk of configuration (or pointer to primitive value) is no longer valid and should not show up in the returned config.Value.

@pietern
Copy link
Contributor Author

pietern commented Nov 3, 2023

Good point! It comes close but is not the same. Two differences come to mind:

  1. Merge is additive and here we use zero-valued fields (primitive of nil pointers) to remove configuration.
  2. By passing the original config.Value to convert.FromTyped we can retain location information for all values that were present in the original. Going from config.Value to Go structs and back is lossy and with this we can avoid loss for values that didn't change.

@pietern pietern added this pull request to the merge queue Nov 15, 2023
Merged via the queue into main with commit 2c908f8 Nov 15, 2023
4 checks passed
@pietern pietern deleted the bundle-from-typed branch November 15, 2023 09:27
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 this pull request may close these issues.

3 participants