types changes for 1.0.0 #783
Conversation
dcbw
force-pushed
the
100
branch
2 times, most recently
from
fd6dbbb
to
3d29b0b
Compare
|
@containernetworking/cni-maintainers no longer WIP, please review. Thanks! |
dcbw
force-pushed
the
100
branch
2 times, most recently
from
843df8a
to
5e8469c
Compare
pkg/types/020/types.go
Outdated
| func copyIPConfig(from *IPConfig) *IPConfig { | ||
| if from == nil { | ||
| return nil | ||
| } | ||
| return from.Copy() | ||
| } |
There was a problem hiding this comment.
I'm not very clear why this function exists. The if nil bit could go in IPConfig.Copy.
There was a problem hiding this comment.
@bboreham can't do that, becuase .Copy() is a struct method, and if you can't do <nil>.Copy() right?
There was a problem hiding this comment.
I haven't tried it, but I can't currently see why not. In Go, nil can have a type, but I think all the calls to this one would be statically dispatched.
pkg/types/current/types.go
Outdated
|
|
||
| var SupportedVersions = []string{"0.3.0", "0.3.1", ImplementedSpecVersion} | ||
| var SupportedVersions = []string{ImplementedSpecVersion} |
There was a problem hiding this comment.
Is this right? Looks like we would still support 0.3.x and 0.4.0.
There was a problem hiding this comment.
No, because 1.0.0 doesn't have the "Version" field in the IPConfig struct and perhaps we'll make other changes before final.
There was a problem hiding this comment.
Follow-up: Bryan was really asking why the plugins export this at all; there are only two uses in the reference plugins and those are for error-checking and can be trivially replaced.
Moving to spec 1.0.0 requires more complicated conversions, so put the converter determination logic in a separate module that any of the types can call to convert between arbitrary versions. This is necessary to ensure the types don't have import cycles. This also implements downconversion, which we never claimed *not* to support, but didn't implement. It also verifies that the Result object unmarshalled by each result type's NewResult() function is actually supported by the result type. This is a potentially breaking change as this was not previously done, and for example may now fail attempts to read a cached PrevResult written by a previous version. Signed-off-by: Dan Williams <dcbw@redhat.com>
dcbw
force-pushed
the
100
branch
2 times, most recently
from
839d9b0
to
dbe3a52
Compare
Only change this time is removal of the IPConfig "Version" field which was deemed redundant. Signed-off-by: Dan Williams <dcbw@redhat.com>
The testcase checking if a prevResult was invalid is not actually useful, because it was testing that an empty prevResult could be unmarshalled. But due to an oversight, prevResults may not have a CNIVersion key, which is all we can check for validity. Signed-off-by: Dan Williams <dcbw@redhat.com>
…esses Redundant and easily determined from the address itself. Signed-off-by: Dan Williams <dcbw@redhat.com>
github.com/containernetworking/cni/pkg/types/current has moved to github.com/containernetworking/cni/pkg/types/040 See containernetworking/cni#783
Moving to spec 1.0.0 requires more complicated conversions, so
put the converter determination logic in a separate module that
any of the types can call to convert between arbitrary versions.
This is necessary to ensure the types don't have import cycles.
This also implements downconversion, which we never claimed
not to support, but didn't implement.
It also verifies that the Result object unmarshalled by
each result type's NewResult() function is actually supported
by the result type. This is a potentially breaking change as
this was not previously done, and for example may now fail
attempts to read a cached PrevResult written by a previous
version.
Signed-off-by: Dan Williams dcbw@redhat.com