fix: use kopsbase.Version instead of kopsbase.KOPS_RELEASE_VERSION#17658
fix: use kopsbase.Version instead of kopsbase.KOPS_RELEASE_VERSION#17658k8s-ci-robot merged 2 commits intokubernetes:masterfrom
Conversation
|
/wip I'm not 100% sure this is right (though I think it is), and it is currently failing tests |
|
The |
|
Looks like etcd is failing to start, but only in some tests... not sure I understand what's going on so let's see if it is consisten /retest |
3c84277 to
23bf6ae
Compare
7b76d82 to
a7429c6
Compare
3a1e73b to
d99f13d
Compare
|
/retest A bunch of pod pending timeouts, and looks like |
|
/test all |
|
Shall we discuss tomorrow in office hours? It was a little tricky to get all the pieces lined up, but I think it's right :-) |
Sure, sounds good. Cya tomorrow! |
| func ImageTag() string { | ||
| tag := Version | ||
| // We replace + with - so that we can use the tag in docker image tags | ||
| return strings.ReplaceAll(tag, "+", "-") |
There was a problem hiding this comment.
Do we want + replaced by - or by _ as in the comment?
There was a problem hiding this comment.
So along the way I think I changed my mind a few times, I can't remember if it was because I actually found somewhere where - was valid and _ was not, but in any case I was not consistent.
I have now fixed this to be - consistently, including in the comments (thanks!). We could try _ if we think that is clearer ... I think in practice it only affects CI builds so it should be easy to switch.
| dest["KopsFeatureEnabled"] = tf.kopsFeatureEnabled | ||
| dest["KopsVersion"] = func() string { return kopsroot.KOPS_RELEASE_VERSION } | ||
| dest["KopsVersion"] = func() string { return kopsroot.Version } | ||
| dest["ImageTag"] = func() string { return kopsroot.ImageTag() } |
There was a problem hiding this comment.
nit: ImageTag may be too generic. Maybe KopsVersionImageTag would be more explicit, same as KopsVersionForLabel.
| dest["KopsVersionForLabel"] = func() string { | ||
| // Labels follow strict rules: a valid label must be an empty string or consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character | ||
| // By convention we use a v prefix here | ||
| return "v" + strings.ReplaceAll(kopsroot.Version, "+", "-") |
There was a problem hiding this comment.
nit: Same as above, do we want + replaced by - or by _?
There was a problem hiding this comment.
We want consistency, I think. I went with - but happy to try _ if you think it's better.
|
This is a significant improvement to versioning and I like the direction. Feel free to /approve and merge if the comments are not relevant. |
Normally they are the same, but for CI builds they are different, and kopsbase.Version is the consistent one to use for CI builds. We also need to be careful not to conflate the version with the docker image tag; image tags cannot contain '+' characters, but our CI versions do. We replace '+' with '_' for image tags. Co-authored-by: Ciprian Hacman <ciprian@hakman.dev>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hakman The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Normally they are the same, but for CI builds they are different,
and kopsbase.Version is the consistent one to use for CI builds.