-
Notifications
You must be signed in to change notification settings - Fork 64
🌱 Allow override of go-verdiff result via label #1964
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
/approve |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1964 +/- ##
=======================================
Coverage 66.78% 66.78%
=======================================
Files 75 75
Lines 6337 6337
=======================================
Hits 4232 4232
Misses 1841 1841
Partials 264 264
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Why? Is the issue here that the golang version went up, not that it threatened to exceed our high-water mark? (in other words, is the check enforcing the wrong thing?) |
There are times we want to be able to override this check. For example, we can go up to golang 1.23.6 as of right now without any downstreaming challenges. But we can't do that if the test is required and always fails. That would mean making the test non-required, and I'm not sure we want to do that and allow something we don't want to slip through. But yes, there may be bingo tools and other updates that can be permitted, and we should allow that EXPLICIT override. |
Aside from vendoring challenges, this project is concerned with coordinating the golang version (including bingo'd tools) to not grow. That isn't really the right goal, IMO, since growth under a threshold should not be problematic. It's when this project (and projects which include its packages) advances a version collectively.
Edit: we use the root go.mod as reference, but I still don't see the value in erroring when any go version changes. |
I'd delete lines 63-78. The failures we're seeing in the kustomize bump are where the golang version changed but still lies at/below the threshold. |
We already use the root |
It's not discarded if it was previously committed. |
That's what I meant. |
hack/tools/check-go-version.sh
Outdated
GO_VER=$(sed -En 's/^go (.*)$/\1/p' "go.mod") | ||
OLDIFS="${IFS}" | ||
IFS='.' MAX_VER=(${GO_VER}) | ||
IFS="${OLDIFS}" | ||
ROOT_GO_MOD="./go.mod" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: define GO_VER after ROOT_GO_MOD, and change it to
GO_VER=$(sed -En 's/^go (.*)$/\1/p' ${ROOT_GO_MOD})
Rather than disabling the go-verdiff CI to allow it to pass when there is a legitimate golang version change, use a label to override the test results. The label is `(override-go-verdiff)`. Signed-off-by: Todd Short <[email protected]>
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: grokspawn, tmshort 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 |
Rather than disabling the go-verdiff CI to allow it to pass when there is a legitimate golang version change, use a label to override the test results.
The label is
(override-go-verdiff)
.Description
Reviewer Checklist