fix(ci): add error handling for GitHub API in upgrade-test.yml. #12876
fix(ci): add error handling for GitHub API in upgrade-test.yml. #12876Priyanshu-u07 wants to merge 1 commit intokubeflow:masterfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @Priyanshu-u07. Thanks for your PR. I'm waiting for a kubeflow 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. DetailsInstructions 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.
Pull request overview
Adds explicit failure handling to the “Get last release tag” step in the upgrade test GitHub Actions workflow so the pipeline fails early with a clear error when the GitHub Releases API cannot provide a valid tag.
Changes:
- Wrap GitHub Releases API fetch in a 3-attempt retry loop with delay.
- Validate
tag_nameis non-empty and not"null", otherwise emit::error::and exit non-zero.
10d0c18 to
986e5be
Compare
jeffspahr
left a comment
There was a problem hiding this comment.
Code Review: Error handling for GitHub API in upgrade-test.yml
Overview
Adds retry logic (3 attempts, 5s delay), response validation, and auth to the "Get last release tag" step in the upgrade test workflow. Previously, a failed API call would silently produce an empty/null tag, causing a confusing downstream kubectl failure.
Assessment: Looks good overall
This is a sensible, low-risk improvement. A few minor notes:
Minor Issues
-
-L(follow redirects) was dropped. The original curl flags were-sSL; the new version uses-sS. While the GitHub releases API endpoint is unlikely to redirect, the-Lflag was there for a reason and removing it is an unnecessary behavioral change. Add it back:curl -sSL --fail-with-body --connect-timeout 10 --max-time 60 \ -
Pipeline exit code masking. The construct:
lastRelease=$(curl ... | jq -r .tag_name 2>/dev/null || echo "")If
curlsucceeds but returns an error JSON body (e.g. rate limit response{"message":"API rate limit exceeded"...}),jq -r .tag_namewill outputnull(since there's notag_namefield), which is caught by the validation. So this works correctly — just noting it's not immediately obvious. -
Auth token is a good addition. Using
${{ github.token }}raises the rate limit from 60/hr (unauthenticated) to 1,000/hr, which addresses the most likely failure mode. This is the most impactful part of the PR.
Nits
- The PR title says
test:but the conventional commit format in this repo typically useschore:orfix(ci):for workflow changes. Looking at the repo's commit history,fixorchorewould be more consistent for CI fixes.
Verdict
Clean, focused improvement. Add back the -L flag and this is good to go.
Signed-off-by: Priyanshu-u07 <connect.priyanshu8271@gmail.com>
986e5be to
bc87c11
Compare
|
New changes are detected. LGTM label has been removed. |
|
@jeffspahr I have addressed the feedback and updated the PR title. |
Description of your changes:
The "Get last release tag" step in [upgrade-test.yml] calls the GitHub API with no error handling. If the API fails (rate limiting, timeout, or outage),
lastReleasebecomes empty or"null", causing the deploy step to fail with a confusing Kubernetes error.This PR adds:
"null"responses::error::annotation andexit 1on failureFixes #12875
Checklist: