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.

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

tests.e2e.ansible: Fix to increase github rate limiting for kustomize #398

Merged
merged 1 commit into from
Jul 24, 2024

Conversation

GabyCT
Copy link
Contributor

@GabyCT GabyCT commented Jul 19, 2024

This PR authenticates for github to increase the threshold for rate limiting while installing kustomize and avoid the random failures that are impacting several CIs.

@GabyCT
Copy link
Contributor Author

GabyCT commented Jul 19, 2024

I tried this PR locally on the TDX machine putting the installation of kustomize in a for loop and seems to be working

@GabyCT
Copy link
Contributor Author

GabyCT commented Jul 19, 2024

@wainersm is there a way to test this on tdx and other CIs?

@@ -39,7 +39,7 @@
ignore_errors: yes
- name: Install kustomize
shell: |
curl -s --retry 3 --retry-delay 10 "https://raw.githubusercontent.com/kubernetes-sigs/kustomize/master/hack/install_kustomize.sh" | bash
curl -s --retry 3 --retry-delay 10 -u ${GITHUB_USER}:${GITHUB_TOKEN} "https://raw.githubusercontent.com/kubernetes-sigs/kustomize/master/hack/install_kustomize.sh" | bash
Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC the GITHUB_USER and GITHUB_TOKEN is not set by default in the CI workflows and one has to specifically set it via env: https://docs.github.com/en/rest/authentication/authenticating-to-the-rest-api?apiVersion=2022-11-28 or am I wrong? (note curl will happily accept -u : so it should work well with empty user as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ldoktor locally if I put echo $GITHUB_USER and $GITHUB_TOKEN seems to exists on the TDX machine

Copy link
Contributor

Choose a reason for hiding this comment

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

Well it's not in https://docs.github.com/en/actions/learn-github-actions/variables#default-environment-variables so might I ask you to add it to the workflows as well to ensure it's going to work properly on all workers?

@GabyCT GabyCT force-pushed the topic/increaserate branch 2 times, most recently from 1fbe65a to a98ee52 Compare July 23, 2024 17:29
@GabyCT
Copy link
Contributor Author

GabyCT commented Jul 23, 2024

Is this going to test the modification of the workflow GHA yaml?

@GabyCT GabyCT force-pushed the topic/increaserate branch 2 times, most recently from 3f6d2f4 to 5dc2ff9 Compare July 23, 2024 22:56
This PR authenticates for github to increase the threshold for rate
limiting while installing kustomize and avoid the random failures
that are impacting several CIs.

Signed-off-by: Gabriela Cervantes <[email protected]>
Copy link
Contributor

@ldoktor ldoktor left a comment

Choose a reason for hiding this comment

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

Thanks, lgtm now. I was wondering a bit about the ${USER} but I found out github ignores it whatsoever and simply uses token (in documentation they actually mention one can use Bearer or token but I tried random characters and it's perfectly happy with it. Another good surprise for me was github.token, I always used secrets.GITHUB_TOKEN but it seems to be on both places.

@GabyCT
Copy link
Contributor Author

GabyCT commented Jul 24, 2024

Thanks, lgtm now. I was wondering a bit about the ${USER} but I found out github ignores it whatsoever and simply uses token (in documentation they actually mention one can use Bearer or token but I tried random characters and it's perfectly happy with it. Another good surprise for me was github.token, I always used secrets.GITHUB_TOKEN but it seems to be on both places.

@ldoktor thanks for all the feedback, I appreciate it

Copy link
Member

@wainersm wainersm left a comment

Choose a reason for hiding this comment

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

Thanks @GabyCT !

@wainersm wainersm merged commit 25429ff into confidential-containers:main Jul 24, 2024
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants