-
Notifications
You must be signed in to change notification settings - Fork 502
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
Set GitHub release as latest if target release is highest semver #3559
Conversation
Welcome @embik! |
Hi @embik. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/cc @xmudrii |
can we add tests? |
/ok-to-test |
a1a48b6
to
f66eeeb
Compare
Sure thing. I'll need to figure out where I can place tests since the |
f66eeeb
to
c81d2bd
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.
@cpanato I looked into adding tests together with @embik and I don't think it's doable at the moment. This package doesn't follow our usual pattern of using mock-able interfaces, so writing tests for it would require significant effort, mainly to refactor the whole package to adhere to the pattern that we use elsewhere. This is also a bit though situation, even if @embik adds tests, the actual calls to GitHub will be mocked and we wouldn't know if it works properly. We should try to do some coordinated test in production, e.g. try this for some release that's not that much time sensitive. I propose accepting this without tests once we're happy with the code and creating a follow up issue to refactor the package and write tests for it. |
thanks |
I created #3565 to follow up on refactoring the package |
c81d2bd
to
38debd0
Compare
38debd0
to
e3c4a68
Compare
Signed-off-by: Marvin Beckers <mail@embik.me>
e3c4a68
to
aa5f62d
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.
After doing extensive rubber ducking session with Marvin, this looks fine to me. Thank you very much for bearing with my comments, and for this contribution of course!
/lgtm
/approve
/hold
@cpanato Are we good to merge this with tests to follow up later on?
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.
if this wil not impact in the existing workflow i am fine
/lgtm
/hold @xmudrii lift when you are ready
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cpanato, embik, xmudrii 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 |
Thank you once again, @embik! |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR attempts to implement heuristics to determine if a GitHub release created by
krel
(and other tools usingpkg/announce
?) should be marked as "latest". It's an overdue follow-up to the extension of release-sdk in kubernetes-sigs/release-sdk#288 that added a field to mark a GitHub release as latest.The idea here is: Unless a release is a pre-release, we start with the assumption that "our" release is the latest one. Since we already query the first page of GitHub releases for determining if this is an existing or new release, my changes now extend that loop and compare our version to the already existing releases by doing a semver parse and comparison.
As soon as we have determined our release is not the latest, this check will be skipped.
Since there doesn't seem to be an easy way to test
krel
, I think this makes sense but I hope it gets scrutinised appropriately during review.Which issue(s) this PR fixes:
Towards kubernetes/kubernetes#119472
Special notes for your reviewer:
Does this PR introduce a user-facing change?