fix(ci): make unit-tests gate poll tolerant of transient API errors#1852
Draft
anurag-atlan wants to merge 1 commit into
Draft
fix(ci): make unit-tests gate poll tolerant of transient API errors#1852anurag-atlan wants to merge 1 commit into
anurag-atlan wants to merge 1 commit into
Conversation
anurag-atlan
requested review from
Aryamanz29,
OnkarVO7,
atlan-ci,
cmgrote,
louisnow and
vaibhavatlan
as code owners
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Contributor
📜 Docstring Coverage ReportRESULT: PASSED (minimum: 30.0%, actual: 77.3%) Detailed Coverage Report |
Contributor
📚 Capability manifest drift detected
To resolve, pick one:
This check is informational, not blocking. Reviewers should ensure the manifest is |
Contributor
📦 Trivy Vulnerability Scan Results
Report SummaryCould not generate summary table (data length mismatch: 9 vs 8). Scan Result Detailsrequirements.txtuv.lock |
Contributor
📦 Trivy Secret Scan Results
Report SummaryCould not generate summary table (data length mismatch: 9 vs 8). Scan Result Detailsrequirements.txtuv.lock |
Collaborator
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified FilesNo covered modified files...
|
Contributor
|
🛠 Full Test Coverage Report: https://k.atlan.dev/coverage/application-sdk/pr/1852 |
Collaborator
🧪 SDR Integration Tests (testcontainer) — mysqlStatus: Failed ❌ What this validates
Re-runPush a new commit, or re-run from the Actions tab. |
Collaborator
|
@sdk-review |
Contributor
|
🔍 SDK Review (mothership) triggered by @vaibhavatlan at 2026-05-29T11:07:56.833Z. Watch the workflow run live — the review summary will appear as a separate comment when complete (typical: 5–30 min, hard cap 2h). ✅ Completed — status |
Contributor
Test SDK Review (mothership): PR #1852 — fix(ci): make unit-tests gate poll tolerant of transient API errorsVerdict: READY TO MERGE
Findings
Holistic Recommendations
Strengths
Note on CISeveral pre-existing checks are red on this PR ( CI: 4 failing (pre-existing, unrelated to this change) |
![mothership-ai[bot]](https://avatars.githubusercontent.com/in/2372052?s=60&v=4){kind=small}
| RESULT=$(gh run view "${RUN_ID}" --repo "${REPO}" --json status,conclusion,url) | ||
| RESULT="" | ||
| for attempt in 1 2 3; do | ||
| if RESULT=$(gh run view "${RUN_ID}" --repo "${REPO}" --json status,conclusion,url 2>&1); then |
Contributor
There was a problem hiding this comment.
[Important] [CORRECTNESS] — 2>&1 merges stderr into RESULT on success, which can defeat the resilience this PR is adding.
Evidence: RESULT=$(gh run view "${RUN_ID}" --repo "${REPO}" --json status,conclusion,url 2>&1)
The if RESULT=...; then break only checks the exit code. If gh exits 0 but also wrote anything to stderr (update notice, deprecation warning, future flag warning, etc.), RESULT contains mixed <stderr text> + JSON. The subsequent STATUS=$(echo "${RESULT}" | jq -r '.status') then fails with a parse error, and because set -euo pipefail is active (line 310), the script exits — killing the gate on a successful gh call. That's the exact failure mode this PR is trying to eliminate.
In practice this rarely fires (gh is usually quiet on --json), but the regression risk lives forever.
Path Forward (immediate fix): capture stderr separately so RESULT only ever holds JSON on success, and only the failure path sees the error text:
ERR_FILE=$(mktemp)
RESULT=""
for attempt in 1 2 3; do
if RESULT=$(gh run view "${RUN_ID}" --repo "${REPO}" --json status,conclusion,url 2>"${ERR_FILE}"); then
break
fi
echo " iteration ${i} attempt ${attempt}/3: gh run view failed: $(cat "${ERR_FILE}")"
RESULT=""
sleep 5
done
rm -f "${ERR_FILE}"Or, if you don't care about prefixing the error with the iteration label, drop 2>&1 entirely — gh's stderr will still surface in the GitHub Actions log just below the iteration line, which is good enough for debugging.
atlan-ci
previously approved these changes
May 29, 2026
Collaborator
atlan-ci
left a comment
atlan-ci
left a comment
There was a problem hiding this comment.
SDK reviewer's verdict: READY TO MERGE.
Full review summary is in the comment posted on this PR.
mothership-ai
Bot
added
the
test-sdk-review-approved
Collaborator
|
Ignore above review. Just testing |
atlan-ci
dismissed
their stale review
Human activity by @vaibhavatlan (issue_comment) detected on this PR — the @sdk-review bot's auto-approval has been dismissed. Re-comment @sdk-review once the PR is ready for the bot to re-approve, or have a code owner approve manually.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Make the `unit-tests-gate` poller resilient to transient GitHub API errors so a single `gh run view` hiccup doesn't kill publish on a passing test.
Problem
The poller has `set -euo pipefail` and runs `gh run view` once per 25s iteration. Any non-zero exit — 401 "Bad credentials", 5xx, transient network blip — kills the whole step. Worse, the warn-only safety net at the next step (`if: steps.wait.outputs.conclusion != 'success'`) doesn't fire when the wait step itself errors out, so all downstream publish jobs cascade-skip via their `!failure()` guards. Result: warn-only mode silently becomes fail-closed on infra blips.
Real incident
Run `atlanhq/atlan-databricks-app/actions/runs/26329578246`:
Tests passed; the gate blocked publish on a one-off API blip.
Fix
Wrap `gh run view` in a 3-attempt retry with 5s backoff inside each polling iteration. If all retries fail in one iteration, log and `continue` to the next 25s tick rather than failing the step. The 30-minute outer cap (`for i in $(seq 1 70)`) is unchanged, so eternal API outages still time out cleanly.
Test plan
Follow-ups (not in this PR)
The broader warn-only semantic issue still stands: even with this retry, an exhausted-retry exit `1` would still cascade-skip publish. Worth a separate change to either (a) write `conclusion=unknown` on retry exhaustion and `exit 0`, letting the warn-only annotate step decide, or (b) change the warn-only step's `if:` to `always()` and tighten downstream `needs:` conditions. Punting that to a follow-up so this fix is small and focused.