-
Notifications
You must be signed in to change notification settings - Fork 5
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
feat: add update-collection-v3 boilerplate #241
Conversation
There is a PR pending that would add that functionality but unfortunately `go-yaml` | ||
doesn't seem to be actively maintained. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to use not actively maintain package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the only one I found with a reasonable number of Github stars (obviously important).
There's also this one as an alternative.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant this one: https://github.com/goccy/go-yaml.
Perhaps this would be a good choice given that it allows parsing yaml using path: https://github.com/goccy/go-yaml#5-use-yamlpath
func parseValues(path string) (ValuesV2, error) { | ||
f, err := os.Open(path) | ||
if err != nil { | ||
return ValuesV2{}, fmt.Errorf("cannot open file %s: %w", path, err) | ||
} | ||
|
||
decoder := yaml.NewDecoder(bufio.NewReader(f)) | ||
|
||
var valuesV2 ValuesV2 | ||
if err := decoder.Decode(&valuesV2); err != nil { | ||
return ValuesV2{}, fmt.Errorf("cannot unmarshal data from %s: %w", path, err) | ||
} | ||
|
||
return valuesV2, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens for partial values.yaml
or for values.yaml
with additional keys?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by partial?
Additional ( if I understand it correctly by not defined in the struct) ones will be dropped. Maybe this could be worked around using yaml.Node
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by partial?
with missing keys, e.g.:
sumologic:
accessId: xxx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is nothing missing in the yaml snippet you posted. There is no notion of "required" in this package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my doubt is, that Stringifying ValuesV2
will override defaults with empty strings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so I've checked this and yes, it will print all the values - also the empty ones.
The nuance here is that if we'd use omitempty
then we couldn't e.g. override a default boolean true
in our chart because false
wouldn't be serialized. Even adding omitempty
wouldn't help because then we'd get all the values.
EDIT: changed field to be pointer fields and added omitempty
. This should do the trick.
385c6e4
to
1fa2762
Compare
LoadConfigFile *bool `yaml:"load_config_file,omitempty"` | ||
} `yaml:"cluster,omitempty"` | ||
CollectionMonitoring *bool `yaml:"collectionMonitoring,omitempty"` | ||
PodLabels struct{} `yaml:"podLabels,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it deserialize:
podLabels:
label1: value1
correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. This will probably have to be either defined as struct or as map[string]interface{}
.
Given this is just a boilerplate PR I'll leave this for future PRs to decide and add tests for it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH I am not sure now if typed language is good for the migration case
On the other hand we will have a typed schema for our collection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have mixed feelings about using go
for this case, but let's see where it will get us
1fa2762
to
a253927
Compare
No description provided.