-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
🐛 Verify that there is a release for the tag #10220
Conversation
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.
Thanks for trying to tackle this! The issue with this PR is it brings us back to a situation where we could be rate-limited by github. Using goproxy was introduced to avoid that rate-limiting. We definitely faced this issue in the CI as clusterctl makes multiple calls to github.
What do you think of instead trying the approach in #7889 (comment) i.e. falling back to an older version if the latest doesn't exist?
Ack to what @killianmuldoon wrote. Note: setting |
5e89d70
to
6f56226
Compare
I did it this way: we get a list of versions, sort them and then iterate and try to get metadata.yaml file for each. If the file exists, we return the latest released version. |
77039e0
to
d05920c
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.
One nit, otherwise looks solid to me.
|
||
return latestPrereleaseTag, nil | ||
// Limit the number of searchable versions by 3. | ||
versionCandidates = versionCandidates[:min(3, len(versionCandidates))] |
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 act as a fallback scenario. Latest version is in a partial state, fallback to previous latest.
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.
@richardcase might have some more context in relation to #7889 (comment) which could impact the desired size here.
In general though I'm fine with the number being larger - providers should be able to test separately to figure out if their releases are accessible.
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.
With the recent failed CAPA releases we went from v2.3.1 to v2.3.5....so we had 3 failed releases.
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 bumped the number to 5. I expect it is enough for everyone :)
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.
Also for extreme cases now it's possible to customize the value with CAPI_CLUSTERCTL_MAX_VERSION_CANDIDATES env variable
d05920c
to
560b896
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
LGTM label has been added. Git tree hash: d1184bc7d717b6daeeb5c62dc8e27b376d59eb27
|
/assign @fabriziopandini @chrischdi |
Overall seems okay to me, but not very familiar with this area of our code |
👍 I also like the change, last nit. I'm not a big fan of having that env variable deep down in the code, but passing it through everywhere also seems odd. |
Also please document the new env var in |
Agree. Do we really need this configurable? What use case are we trying to cover with this? I would prefer not adding another config env var if not strictly necessary |
That config variable was requested by CAPA team and @richardcase. Recently they had a situation with 4 broken tags, and potentially there could be more. So, to handle such issues we always want to provide a workaround that allows users to set a bigger number of tags to search. |
In general I would like to try to keep the number of configuration options to a minimum. And to be honest environment variables are one of the worst ways to configure a CLI :). Can we pick a value which is high enough for the cases that occur in 99% of the cases. Maybe 10? (@richardcase ?) (If we pick 10, I assume the 10 is only fully used when the last 10 existing patch tags for a minor are broken, as soon as there was a successful release it's not relevant anymore, correct?) |
Imo 5 tags is more than enough. Normally there is only one broken tag, which is not yet released. In some cases there could be 2. |
Now we attempt to retrieve provider release data first via the go-proxy client. In case of failure, we would resort to direct polling from GitHub. However, the go-proxy client only lists existing git tags and does not return detailed information about github releases. This behavior leads to a failure in clustercl if there's no release for the latest tag. To address this, we try to get a release for the tag first, and return the highest version with a corresponding release. For the sake of performance we do only 5 attempts to find a release, and return an error after that.
57f3fee
to
1c2aa6c
Compare
Thank you very much for your work on this! /lgtm /assign @chrischdi @fabriziopandini |
/cherry-pick release-1.7 |
LGTM label has been added. Git tree hash: 3f130245e191bd8927e988d4e2bd62bbbaca1440
|
@sbueringer: once the present PR merges, I will cherry-pick it on top of release-1.7 in a new PR and assign it to you. In response to this:
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. |
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
Thanks for contributing this!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chrischdi 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 |
@sbueringer: new pull request created: #10423 In response to this:
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. |
/cherry-pick release-1.6 |
@sbueringer: #10220 failed to apply on top of branch "release-1.6":
In response to this:
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. |
What this PR does / why we need it:
Now we attempt to retrieve provider release data first via the go-proxy client. In case of failure, we would resort to direct polling from GitHub. However, the go-proxy client only lists existing git tags and does not return detailed information about github releases. This behavior leads to a failure in clustercl if there's no release for the latest tag.
To address this, we try to get a release for the tag first, and return the highest version with a corresponding release.
For the sake of performance we do only 5 attempts to find a release, and return an error after that.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #7889
/area clusterctl