cluster-api
cluster-api copied to clipboard
kubernetes-sigs
🐛 Verify that there is a release for the tag
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 performace we do only 3 attempts do 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
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 GOPROXY=off allows to turn off the goproxy functionality.
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. I think it is a good approach, because we download this file anyway, and during the first attempt it will be cached.
LGTM label has been added.
/assign @fabriziopandini @chrischdi Could be interesting to get in. To see if it works for the upcoming minor release
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 docs/book/src/clusterctl/overview.md
👍 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.
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.
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. Okay, I'll remove this configuration for now. We can always add it if necessary.
Thank you very much for your work on this!
/lgtm
/assign @chrischdi @fabriziopandini
LGTM label has been added.
@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:
/cherry-pick release-1.7
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.
[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
- ~~OWNERS~~ [chrischdi]
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
@sbueringer: new pull request created: #10423
In response to this:
/cherry-pick release-1.7
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.
@sbueringer: #10220 failed to apply on top of branch "release-1.6":
Applying: fix: verify that there is a release for the tag
Using index info to reconstruct a base tree...
M cmd/clusterctl/client/repository/repository_github_test.go
Falling back to patching base and 3-way merge...
Auto-merging cmd/clusterctl/client/repository/repository_github_test.go
CONFLICT (content): Merge conflict in cmd/clusterctl/client/repository/repository_github_test.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 fix: verify that there is a release for the tag
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
In response to this:
/cherry-pick release-1.6
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.