Skip to content
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.

Sign up for GitHub

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

Jump to bottom

🌱 use http get to download files from GitHub in clusterctl #9236

Merged
merged 1 commit into from
Aug 28, 2023
Merged

🌱 use http get to download files from GitHub in clusterctl #9236

merged 1 commit into from
Aug 28, 2023

Conversation

fabriziopandini
Copy link
Member

What this PR does / why we need it:
Remove API calls to the rate-limited GitHub API

Which issue(s) this PR fixes:
Rif ##8426

/area clusterctl

@k8s-ci-robot k8s-ci-robot added area/clusterctl Issues or PRs related to clusterctl cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 17, 2023
@fabriziopandini fabriziopandini mentioned this pull request Aug 17, 2023
Closed
@fabriziopandini fabriziopandini force-pushed the clusterctl-httpget-files-from-github branch from 4fe82f3 to 24c4d18 Compare August 18, 2023 06:34
@fabriziopandini fabriziopandini force-pushed the clusterctl-httpget-files-from-github branch 2 times, most recently from ddaf5be to 528bbd8 Compare August 18, 2023 18:19
@fabriziopandini
Copy link
Member Author

Rebased + dropped the custom implementation of following redirect given that http.Get does this natively

chrischdi
chrischdi reviewed Aug 21, 2023
View reviewed changes
Copy link
Member

@chrischdi chrischdi left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

(minus golangclilint)

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 21, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: ab9f40d9216eb46a27efdd23c8ba6d30b0904ac3

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 21, 2023
@k8s-ci-robot k8s-ci-robot requested a review from chrischdi August 21, 2023 09:24
@richardcase
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 21, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 597a60822ddc891ed4fb79281508ff7338ebc25a

@dlipovetsky
Copy link
Contributor

Would we consider cherry-picking this to 1.4? CAPA e2e tests are hitting GitHub rate limits, and this would resolve the issue.

@sbueringer
Copy link
Member

sbueringer commented Aug 25, 2023 •
edited
Loading

Would we consider cherry-picking this to 1.4? CAPA e2e tests are hitting GitHub rate limits, and this would resolve the issue.

I personally don't have a strong opinion on it, but I have a hard time finding a category of allowed backports it would fall into. I'm somewhat hopeful with recent developments that we can get GitHub tokens for CI which means we would not have to backport this fix. Maybe let's see if we can make some progress on the GitHub token? (https://kubernetes.slack.com/archives/C01672LSZL0/p1692945837121219)

@fabriziopandini fabriziopandini force-pushed the clusterctl-httpget-files-from-github branch from 33a4913 to 5c057b4 Compare August 25, 2023 19:05
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 25, 2023
@fabriziopandini
Copy link
Member Author

fabriziopandini commented Aug 25, 2023 •
edited
Loading

addressed the last nits and squashed commits.
I think we need the token no matter this PR, so I think that we should wait a little bit more before force-backporting.

@sbueringer
Copy link
Member

Thank you!

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 28, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: f0a5b79293cb9feae4cf48b4a108596744c3245d

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 28, 2023
@k8s-ci-robot k8s-ci-robot merged commit 735e904 into kubernetes-sigs:main Aug 28, 2023
@k8s-ci-robot k8s-ci-robot added this to the v1.6 milestone Aug 28, 2023
@fabriziopandini fabriziopandini deleted the clusterctl-httpget-files-from-github branch November 17, 2023 10:37
@fabriziopandini fabriziopandini mentioned this pull request Jan 19, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbueringer sbueringer sbueringer left review comments

@jackfrancis jackfrancis Awaiting requested review from jackfrancis

@richardcase richardcase Awaiting requested review from richardcase

@chrischdi chrischdi Awaiting requested review from chrischdi

Assignees

@richardcase richardcase

@chrischdi chrischdi

@sbueringer sbueringer

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/clusterctl Issues or PRs related to clusterctl cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.6
Development

Successfully merging this pull request may close these issues.
6 participants
@fabriziopandini @k8s-ci-robot @richardcase @dlipovetsky @sbueringer @chrischdi