-
Notifications
You must be signed in to change notification settings - Fork 554
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
Update State structure to use the new ContainerState type #1056
Conversation
Signed-off-by: Renaud Gaubert <[email protected]>
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.
LGTM.
I wondered if this would be a breaking change (from a "go" perspective), but thinking about that, I guess the tags in this repository reflect the "spec", not the go API; should this be mentioned somewhere? (or should the "specs-go" get it's own versioning/tags? ( |
LGTM |
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.
LGTM
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.
LGTM
I think we've had the "Go breaking change" conversation previously, and came to the conclusion that the version is for the spec itself, and the Go code here is merely supporting rather than the focus (so "Go breaking changes" are not critical).
Absolutely makes sense. I went looking if this was mentioned anywhere, and couldn't find it in the readme or RELEASES.md. I'm wondering if (given that the Go code and spec don't follow the same "stability" from a SemVer perspective) if versioning should be decoupled. Doing so would also allow more flexible handling of changes desired in the Go code, without requiring a new release of the specification itself. Given that the go code lives in a subdirectory/package, the project is in a "fortunate" position that this would be possible (using the I can open a ticket for discussion |
ready to merge? |
Following up on @kolyshkin 's comment, we realized I should have updated the
State
structure to use that new type.Thanks for picking up on this mistake!
/cc @kolyshkin @titanous
Signed-off-by: Renaud Gaubert [email protected]