.*: add verify script for go directive changes #267
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/M
|
/approve |
k8s-ci-robot
added
the
lgtm
|
/assign @sttts |
| exit 1 | ||
| fi | ||
|
|
||
| if ! printf '%s\n' "${current}" "${max}" | sort --check=silent --version-sort; then |
There was a problem hiding this comment.
Why not test for exact equal? We don't want it to go backwards either, right?
There was a problem hiding this comment.
Trying to understand this a little better, would this serve as a sanity check in general? Because when we bump deps, the go directive is either going to stay the same or its going to increase, I don't think it can go backwards iiuc.
There was a problem hiding this comment.
What I meant is that this is obvious:
if [[ "${current}" != "${max}" ]]; then
echo >&2 "FAIL: current Go directive ${current} must be ${max}"
fi
If I understand this code, you would allow "max" to be 1.20.1 and "current" to be 1.20 - why would we do that? I feel like it's too clever -- don't we ALWAYS want an exact match?
The goal here is that the verify script is not going to be changed "accidentally" (by tooling), so it's an effective (but simple) cross-check.
There was a problem hiding this comment.
Uggh, after making my point, you clever showed me I was being an idiot. This is not "locking" the version, it really is upper-bounding it. As soon as we add support for type-params, 1.13 will have to bump up, I guess?
There was a problem hiding this comment.
Yeah, "upper-bounding" is a nice way to put it. Thanks! <3
As soon as we add support for type-params, 1.13 will have to bump up, I guess?
Yeah, that sounds about right!
This commit adds a script that checks the changed version in the go.mod file with a certain maximum version that the go directive can have. We set the maximum version of the go directive as 1.20 here because the oldest go directive that exists on our supported release branches in k/k is 1.20. Signed-off-by: Madhav Jivrajani <madhav.jiv@gmail.com>
MadhavJivrajani
force-pushed
the
add-go-directive-verify
branch
from
2d939fd to
7d40e07
Compare
k8s-ci-robot
removed
the
lgtm
|
/approve |
k8s-ci-robot
added
the
approved
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dims, MadhavJivrajani, sttts, thockin 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 |
Building on kubernetes/utils#306
This commit adds a script that checks the changed version in the go.mod file with a certain maximum version that the go directive can have.
We set the maximum version of the go directive as 1.20 here because the oldest go directive that exists on our supported release branches in k/k is 1.20.
Fixes #264
xref kubernetes/kubernetes#123744
TODO: need to change the command here: https://github.com/kubernetes/test-infra/blob/0100e558a4a3594165262e0864a53d1b22f0973c/config/jobs/kubernetes/gengo/gengo-config.yaml#L29Done kubernetes/test-infra#32368
/assign @liggitt @dims