-
Notifications
You must be signed in to change notification settings - Fork 1
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
add format_version mutation #3
Conversation
ecs-update/ecs-update.go
Outdated
if formatVersion == "2.0.0" { | ||
pkg.Manifest.OriginalData.License = "" | ||
} | ||
err = WriteDocument(&pkg.Manifest, pkg.Manifest.WriteYAML) |
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 think there are two problems that stem from gopkg.in/yaml not being able to roundtrip content while preserving the format.
- Marshaling the manifest.yml back out results in larger than necessary diffs and possibly loss of comments. I introduced some big hacks (which are probably more complicated than alternative approaches) to edit the YAML content for the other changes that are being made. My hack is not sustainable.
Probably a better approach would be have a consistent YAML formatting applied to the files such that it does not matter if the tool produces a different ordering since we can just sort it again. But the comments being lost is still a problem. Maybeyq
commands could be an option. - My manifest structs do not have all of the possible attributes. So when you marshal the struct back out you lose attributes. This was OK before when I was only reading the attributes that I needed and was editing the original YAML code and yaml.Node. The solution for this is to use structs generated from package-spec.
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've replaced the logic here with a real YAML AST handler so those issues should no longer be problems.
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.
The change to use goccy looks good. I need to update the two hacks I put in to use goccy instead of manual YAML manipulation.
Be aware that goccy/go-yaml has some bugs that make it unsafe with invalid YAML. I would review the issues. |
This adds the capacity to bump the format version.
A number of changes were needed, mainly around the YAML codec (yaml.v3 doesn't respect json struct tags).
Please take a look.
For elastic/integrations#4518