fix: use kopsbase.Version instead of kopsbase.KOPS_RELEASE_VERSION #17658
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
area/api
size/S
k8s-ci-robot
added
size/S
|
/wip I'm not 100% sure this is right (though I think it is), and it is currently failing tests |
k8s-ci-robot
added
area/addons
size/M
|
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 |
justinsb
force-pushed
the
one_version
branch
2 times, most recently
from
3c84277 to
23bf6ae
Compare
k8s-ci-robot
added
size/L
justinsb
force-pushed
the
one_version
branch
4 times, most recently
from
7b76d82 to
a7429c6
Compare
justinsb
force-pushed
the
one_version
branch
2 times, most recently
from
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! |
hakman
requested review from
ameukam and
rifelpet
and removed request for
johngmyers
| 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. |
k8s-ci-robot
added
the
lgtm
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 <[email protected]>
k8s-ci-robot
removed
the
lgtm
k8s-ci-robot
added
the
lgtm
|
[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
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
3 participants
Normally they are the same, but for CI builds they are different,
and kopsbase.Version is the consistent one to use for CI builds.