Support multiple api_versions in the same codebase #228
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
drevell
commented
Oct 10, 2023
drevell
commented
Oct 10, 2023
…o present all the pieces at the same time. This PR creates a system for supporting YAML files using different `api_version`s from a single binary. This is implemented by having a separate set of structs for each `(kind,api_version)` pair. Each api_version can change the semantics or existence of YAML fields. The structs are stored in `templates/model/$KIND/$APIVERSION`. Instructions for adding a new `api_version` are in `templates/model/README.md`, that might be a good starting point for review. Old versions are automatically converted to new versions before being used: old structs define an Upgrade() method that define how to upgrade them. There's a registry of `api_version`s in decode.go that defines which versions exist and which structs are associated with each. To demonstrate that this all actually works, this PR creates a new API version `v1beta1`. The only change in this new version is adding the optional `if` field for each step in spec.yaml (#220). Alternatives considered to try to avoid this huge copy-pasting of structs: - Struct embedding to reduce copy-pasta: this would expose a more complicated API to the rest of the code that has to use the structs. It makes creating a new version more painful. It also triggers golang/go#15924 since we use `reflect.StructOf`. - Use github.com/mitchellh/mapstructure` to avoid so much double-parsing: we'd lose YAML position information, and our users really appreciate getting line numbers on their error messages. - Use `switch(apiVersion)` within a single struct instead of copy-pasting structs: high risk of accidentally changing the behavior of old API versions and generally creating fragile code. I've marked files that are mostly copy-pasted so you can mostly skip reviewing them
drevell
force-pushed
the
drevell/api-versions
branch
from
311a07e
to
45b2890
Compare
sethvargo
reviewed
Oct 10, 2023
verbanicm
reviewed
Oct 10, 2023
verbanicm
reviewed
Oct 10, 2023
sethvargo
approved these changes
Oct 10, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Apologies for the large PR, but I think in this case it's important to present all the pieces at the same time.
This PR creates a system for supporting YAML files using different
api_versions from a single binary. This is implemented by having a separate set of structs for each(kind,api_version)pair. Each api_version can change the semantics or existence of YAML fields. The structs are stored intemplates/model/$KIND/$APIVERSION.Instructions for adding a new
api_versionare intemplates/model/README.md, that might be a good starting point for review.Old versions are automatically converted to new versions before being used: old structs define an Upgrade() method that define how to upgrade them.
There's a registry of
api_versions in decode.go that defines which versions exist and which structs are associated with each.To demonstrate that this all actually works, this PR creates a new API version
v1beta1. The only change in this new version is adding the optionaliffield for each step in spec.yaml (#220).Alternatives considered to try to avoid this huge copy-pasting of structs:
reflect.StructOf.switch(apiVersion)within a single struct instead of copy-pasting structs: high risk of accidentally changing the behavior of old API versions and generally creating fragile code.I've marked files that are mostly copy-pasted so you can mostly skip reviewing them