Add bumper script #1968
Add bumper script #1968
Conversation
kubevirt-bot
added
release-note-none
hack/bumper.sh
Outdated
| git pull upstream "$(git symbolic-ref --short HEAD)" | ||
| go mod edit -dropreplace="${PACK}" | ||
| go mod edit -require="${TARGET_PACK}" | ||
| make vendor |
There was a problem hiding this comment.
| make vendor | |
| make fmt vet |
make fmt already does vendor
make vet checks bump didn't brake anything
There was a problem hiding this comment.
this script is meant to be generic, not just for CNAO,
i am not sure we have on all our repos fmt / vet
We dont even have vendor on all repos, i had a better version that handled this case
for example here we dont have make vendor
https://github.com/k8snetworkplumbingwg/ovs-cni/blob/main/Makefile#L90
The better version knew to check if make vendor exists, otherwise to run go mod tidy / go mod vendor
it also doesnt have fmt and vet named targets (it has make format instead)
so i prefer to not add it now, before we make sure it works all over
whoever needs can just run the additonal commands and amend
more info / options here #1968 (comment)
atm i would prefer to not block due to that
There was a problem hiding this comment.
I think we should fail before the PR is created,
how about adding go vet $WHAT where WHAT is defaulted to ./pkg/...?
There was a problem hiding this comment.
Even if we don't fail and just run later commit amend it is fine basically (of course we can optimize)
i am against adding something like go vet $WHAT, because on some repos we don't run go vet.
What i can do is add make vet if make vet target exists, this way it is generic and robust, as this script is intended
|
found the better script, lets continue from there (will add vet as done already with vendor there) |
Ease go package bumping. Signed-off-by: Or Shoval <[email protected]>
|
|
added the vet |
|
/retest |
| rm -rf build/_output/ | ||
| make vendor | ||
| else | ||
| go mod tidy |
There was a problem hiding this comment.
should we do -compat=${desired_version}?
There was a problem hiding this comment.
i would say not yet
because it was meant for OVS, which doesnt do it as well, so they should be the same
https://github.com/k8snetworkplumbingwg/ovs-cni/blob/main/Makefile#L91
btw what we can do for OVS, is to add make format if exists
https://github.com/k8snetworkplumbingwg/ovs-cni/blob/main/Makefile#L55
which does fmt / vet
There was a problem hiding this comment.
I would say on long term align, and on short term just add all options? wdyt?
There was a problem hiding this comment.
i would suggest please to start simple, as is
and adapt OVS / make sure all the other repos are aligned, and finally change this one, wdyt?
|
|
||
| # ./hack/bumper.sh CVE-2021-38561 golang.org/x/[email protected] release-0.89 | ||
|
|
||
| # to skip semver check (for example in case package is dropped from go.mod after updating) |
There was a problem hiding this comment.
I don't get the scenario where SKIP would be true.
Why would we bump a package, and it would end up getting removed?
There was a problem hiding this comment.
Nice question
for example if there was a static replace, which the script removes
https://github.com/kubevirt/cluster-network-addons-operator/pull/1968/files#diff-958372dc14cde9068a295622a0c8af96ac251e37b948589228f81ac0a7d0da6cR31
and once the script done, the new set doesn't need anymore the old package at all.
This what happened to me, so I added it.
There was a problem hiding this comment.
ah you mean there was a lonely replace without a require?
There was a problem hiding this comment.
there were both (as far as i remember, anyhow it is the same thing, unless there isn't such thing replace without require)
the require was mandatory until some point, because else it conflicted,
but once the package could be updated, the script temporary updated it, removed the replace which enforce specific version, run make vendor, and then the package become unrequired at all given the new go.mod set
(auto removed by vendor it self)
There was a problem hiding this comment.
OK I guess. I just never encountered this scenario before.
|
/lgtm |
|
Thanks |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: oshoval 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 |
kubevirt-bot
added
the
approved
What this PR does / why we need it:
Ease go package bumping.
Special notes for your reviewer:
Release note: