Ensure conformance image version is prefixed with 'v' #717
Conversation
When using flags to set the kube-conformance-image-version the `Set` method ends up stripping the `v` prefix which it requires the user to provide. This causes a problem because the rest of the codebase assumes the version will start with that prefix due to the fact that that is how images are tagged (for Sonobuoy, Heptio kube-conformance, and for upstream conformance images). This change just adds the prefix back after validating the version and updates the tests to actually check for the version that gets persisted. Fixes #716 Signed-off-by: John Schnake <jschnake@vmware.com>
Codecov Report
@@ Coverage Diff @@
## master #717 +/- ##
==========================================
+ Coverage 39.22% 39.38% +0.15%
==========================================
Files 68 68
Lines 3816 3816
==========================================
+ Hits 1497 1503 +6
+ Misses 2221 2217 -4
+ Partials 98 96 -2
Continue to review full report at Codecov.
|
|
|
||
| // It is important to set the string with the `v` prefix in order | ||
| // to be consistent with server version reporting and image tagging norms. | ||
| *c = ConformanceImageVersion(fmt.Sprintf("v%v", version.String())) |
There was a problem hiding this comment.
Should ConformanceImageVersion return the properly formatted string?
There was a problem hiding this comment.
ConformanceImageVersion isn't a function, it is just casting the string to the type.
If you meant that "should the String() string method on the ConformanceImageVersion type do this formatting, I would argue no. We have a constrained type so if we want validation like this we have to either use custom Get() methods which return the value and an error (more overhead than we need for a flag wrapper) or just do the proper validation on Set.
To put all this logic in the String method would be a bit awkward because we wouldn't be able to handle the error case so we'd just end up having to put the logic in both places. And once we fix it here we don't really have a problem which would make it worth copying the logic to String or a custom Get method in addition to what we have.
(tl;dr; the problem really is with the logic itself and not where we put it)
What this PR does / why we need it:
When using flags to set the kube-conformance-image-version
the
Setmethod ends up stripping thevprefix which itrequires the user to provide.
This causes a problem because the rest of the codebase assumes
the version will start with that prefix due to the fact that that
is how images are tagged (for Sonobuoy, Heptio kube-conformance,
and for upstream conformance images).
This change just adds the prefix back after validating the version
and updates the tests to actually check for the version that
gets persisted.
Which issue(s) this PR fixes
Fixes #716
Special notes for your reviewer:
A simple test:
Both of those should result in images with the tag prefixed with
v. In addition, the first should be a heptio image, the latter a google image.As it stands now, the
vprefix is missing and both try to use the Heptio registry due to the missing prefix affecting the comparison.Release note: