feat: store presets as YAML rather than Go#152
feat: store presets as YAML rather than Go#152tonyandrewmeyer merged 12 commits intocanonical:mainfrom
Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
dimaqq
left a comment
There was a problem hiding this comment.
I didn't review, just commenting to show that I really appreciate this change! ❤️
benhoyt
left a comment
There was a problem hiding this comment.
One other comment: I'd expect to see a few minor updates to README.md mentioning this where it talks about presets.
james-garner-canonical
left a comment
There was a problem hiding this comment.
I don't have any major issues with the implementation of the PR feature, though I've left a few suggestions.
Making the channel field optional for snaps seems unrelated to the PR changes, but seems reasonable enough, however the behaviour when using the default seems like it would be subtly different compared to explicitly specifying latest/stable in the case of a snap already being installed with a different channel. This seems OK, but should at least be documented clearly (e.g. "channel is optional, the default behaviour is decided by snapd).
I'm guessing that you validated that (e.g.) defaultK8sConfig evaluates to the same value as we get by loading presets/k8s.yaml, but please confirm this. Were those perhaps the longer tests that got removed?
Approving as none of my suggestions are strictly blocking, though I would like them addressed/rejected before merging :)
It came through earlier discussions and review with Ben. Without that change, the presets are considerably longer, and it's an issue that didn't exist previously because the Go code could use a common method for the various presets. With the YAML, there's no ability to have common handling, so there's a lot more repetition. We felt that removing the need to have
Good point, I'll update the docs.
Yes, the actual presets are the same, and the spread tests mostly verify this as well.
There were unit tests that checked the contents of each preset, yes. To me (and Ben) that felt unnecessary because it was really just duplicating the definition of the preset. |
Thanks for explaining :) |
This PR migrates built-in configuration presets from hard-coded Go structs to embedded YAML files, making presets easier to edit and ship while keeping them available at runtime. This should also make it simpler for users to develop their own config by starting from an appropriate preset.
Changes:
presets/and embedded them into the binary via go:embed.internal/configto enumerate/read presets from the embedded filesystem.channeloptional for snaps, defaulting tolatest/stable.Fixes #103.