Skip to content
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

change spec test from exact match to greater than #406

Closed
wants to merge 1 commit into from

Conversation

mikebrow
Copy link
Member

Current version test checks for an exact match. Changing test to a greater than test instead of forcing folks to edit their config.json version tag each time the container spec version changes. To easy to just change the version.. without editing the broken fields. Might as well just tell folks to gen a new spec and merge than have them manually changing the version in the config.json file without knowing what changed.

Signed-off-by: Mike Brown brownwm@us.ibm.com

Signed-off-by: Mike Brown <brownwm@us.ibm.com>
@wking
Copy link
Contributor

wking commented Nov 17, 2015

On Tue, Nov 17, 2015 at 02:23:30PM -0800, Mike Brown wrote:

Changing test to a greater than test instead of forcing folks to
edit their config.json version tag each time the container spec
version changes.

This should probably use a SemVer library like [1](although I'm not
familiar with this space, maybe another library from [2] would be a
better fit). And I'm not sure how well the current Go types from
opencontainers/specs will handle this sort of flexibility, although it
should be easier if the spec switches to protobuf (e.g. see
compatibility notes in opencontainers/runtime-spec#233).

@hqhq
Copy link
Contributor

hqhq commented Nov 18, 2015

Specs has no guarantee for backward compatibility for now, so I think we better keep checking the exact match.

@marcosnils
Copy link
Contributor

@hqhq my first impression after looking at the spec version was that it's semantically versioned. Which of course implies that you can have backwards compatibility within the same major version. If the spec versioning is still undefined I guess it'd be nice to clarify it somewhere as users might get confused the same way that I did.

@hqhq
Copy link
Contributor

hqhq commented Nov 18, 2015

@marcosnils Hope opencontainers/runtime-spec#253 will help.

@marcosnils
Copy link
Contributor

@hqhq as long as it's true that the 0.x series doesn't provide backwards compatibility then it's ok for me.

@wking
Copy link
Contributor

wking commented Nov 18, 2015

On Wed, Nov 18, 2015 at 01:32:31AM -0800, Qiang Huang wrote:

Specs has no guarantee for backward compatibility for now, so I
think we better keep checking the exact match.

A valid SemVer 2.0.0 comparison should not succeed for 0.x.y
comparisons unless there is an exact match. See point 4 in 1. I'm
not sure how good the various Go SemVer libraries are at implementing
that rule, but if we can find a reliable SemVer comparator we should
still be able to switch to it now (before the spec hits the second 1.x
release and picks up some backwards compatibility requirements).

@mrunalp
Copy link
Contributor

mrunalp commented Nov 18, 2015

Yes, we should stick with exact match for now.

@mikebrow mikebrow closed this Nov 18, 2015
@duglin
Copy link

duglin commented Nov 19, 2015

Keeping an exact match really hurts useability. This means that all existing configs will need to be changed before they can used after an upgrade even if they are perfectly valid and usable. For example, let's say we add a new optional field to the config so we bump the version string, why should we force all container configs to change their version string even if they can still run "as is"?

My preferred approach is to make the version string optional and only generate an error if there's a field in there that the runtime doesn't recognize. As an end-user I would really like to be able to just provide a minimal config (perhaps even with nothing more than the cmd to run) and let everything else default. This way my container can be used by pretty much any implementation of OCI w/o modifications irrespective of what exact version is deployed. If there is a version string, its real only purpose is to clarify how the runtime is to act if there is a change in semantics around a field. We don't need a version string to know whether we understand an particular field - we either have the code for it or we don't. We only need a version string for cases such as if we need the user to let us know if "foo" is meant to act like v1.0's "foo" or v2.0's "foo". But even in those cases, I would prefer to change the name of the field rather than its semantics and we can generate an error if both are specified (which would happen anyway once the old field is no longer supported since it would become an "unknown" field). And yes, I would actually really prefer to just remove it entirely, but I suspect there might be some pushback on that :-)

@mikebrow
Copy link
Member Author

Maybe change this test to a warning informing the user that the version of the config.json is back level and points the user to a location that has a history of changes to the config.json definition/format? Warnings vs. error would be a bit more usable. Then when there are actual changes to the spec that require changes to a particular config.json we could add runtime tests as @duglin has outlined and either throw an error or implement backlevel support? I also see the guys are talking about two modes here... one for now... break it if you change it... and one for later on post some future release where the tests would possibly be more like Doug's proposal.

@wking
Copy link
Contributor

wking commented Nov 19, 2015

On Wed, Nov 18, 2015 at 10:26:38PM -0800, Doug Davis wrote:

Keeping an exact match really hurts useability.

This is what pre-1.0 means. The goal should be to get to 1.0 as fast
as we can, so we can start expecting working backwards compatibility.
On the other hand, we can start practicing post-1.0 SemVer bumps in
the 0.x series, and that should help folks understand when we think
changes are backwards compatible or not 1. I think a bigger
usability problem is that it's not clear which runC versions support
the 0.1.1 spec (which is currently the only spec release 2). More on
this in #277. The current master will (with #405) claim to support
v0.2.0 of the spec 3, but that hasn't even been cut yet.

 Subject: Re: Initial Draft Release
 Message-ID: <20150911205808.GC5912@odin.tremily.us>

@mikebrow mikebrow deleted the loosen-up-spec-test branch February 9, 2016 20:38
stefanberger pushed a commit to stefanberger/runc that referenced this pull request Sep 8, 2017
Fixes opencontainers#398

Signed-off-by: Vincent Batts <vbatts@hashbangbash.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants