-
Notifications
You must be signed in to change notification settings - Fork 25
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
🌱 Increment internal versions to use release v0.4.3 #58
Conversation
e532683
to
1138402
Compare
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.
/lgtm
Thanks!
cc @vincepri for approval |
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.
/lgtm
@@ -9,4 +9,4 @@ runs: | |||
# this is built using GCB by building the Dockerfile in this directory on | |||
# every tagged release. After tagging a release, a new version should | |||
# automatically appear. | |||
image: 'docker://gcr.io/kubebuilder/pr-verifier:v0.4.1' |
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.
I think this should be v0.4.3 now.
We wouldn't want a kube-builder-tools v0.4.3 release that points to a v0.4.2 image?
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.
I'm happy to make the change to v0.4.3 and have done so since it's easy enough to revert - adding @killianmuldoon 's point below this PR should probably only be merged if a 0.4.3 release if cut right after, otherwise the v0.4.3 image won't actually exist.
Only if there's certain to be a release with that version - i.e. this would be a release PR. If we want to use the sha of a commit using the current version with an image that exists is the right approach IMO |
Signed-off-by: Michael Shen <mishen@umich.edu>
Ah I see |
/lgtm @vincepri @camilamacedo86 Can we merge this PR and cut a new v0.4.3 release? So we have a release without the panic and a release which uses the same image (v0.4.3) as the release tag (v0.4.3) |
@@ -7,7 +7,7 @@ replace sigs.k8s.io/kubebuilder-release-tools/notes => ../notes | |||
require ( | |||
github.com/google/go-github/v32 v32.1.0 | |||
golang.org/x/oauth2 v0.8.0 | |||
sigs.k8s.io/kubebuilder-release-tools/notes v0.4.0 | |||
sigs.k8s.io/kubebuilder-release-tools/notes v0.4.2 |
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.
while we can't bump this to non-existent version as we are doing in other file with image version, would not it be problem (not having our fix included in the tool) when using it from CAPI?
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.
This version is not used at all, see the replace in l.5.
Maybe we should just set it to v0.0.0 (if possible) to avoid confusion
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.
@sbueringer I see, have not expanded the file fully to see the replace.. Then it should be all good 👍🏼
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.
Yup. It's just pretty confusing as this version looks like something that almost makes sense, but can never actually be the same version
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.
cc @vincepri PTAL
We (CAPI release team) would appreciate a new release once this is merged, thanks!
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.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mjlshen, vincepri 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 |
@vincepri Thanks for getting this merged in! Could you help get a v0.4.3 release cut as well? Unfortunately the code that got merged won't work without it. |
This will allows users of this GitHub action to use the changes from #56