adding github workflow to check dspa integration test workflow #943
Conversation
WalkthroughAdds a GitHub Actions workflow Changes
Sequence DiagramsequenceDiagram
actor Dev as Developer
participant GH as GitHub
participant Gate as stable-branch-gate.yml
participant Up as Upstream Repo
Dev->>GH: Open / update PR targeting `stable`
GH->>Gate: Trigger workflow (PR events)
rect rgb(210,230,250)
Note over Gate,Up: check-remote-workflow-status
Gate->>Up: Query `stable-merge-check.yml` runs for mapped branch
Up-->>Gate: Return workflow runs
Gate->>Gate: Filter completed & successful runs
Gate-->>GH: Emit outputs (status, message, workflow_url)
end
rect rgb(230,240,220)
Note over Gate,GH: set status check
Gate->>GH: Create commit status on PR head SHA (context: stable-branch-gate/remote-integration-tests)
GH-->>Gate: Acknowledge status
end
rect rgb(250,235,220)
Note over Gate,GH: provide-instructions (on PR open)
Gate->>GH: Post PR comment with branch protection & upstream test instructions
GH-->>Gate: Comment posted
end
GH-->>Dev: Show status check and PR comment
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: Nelesh Singla <[email protected]>
nsingla
force-pushed
the
stable-merge-check
branch
from
1588057 to
87cf646
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/stable-branch-gate.yml (3)
81-81: Fix indentation inconsistency.Line 81 has irregular indentation with extra leading spaces that may cause parsing issues.
} else { console.log('❌ No workflow runs found for this branch'); core.setOutput('status', 'failure'); core.setOutput('message', 'No remote integration test runs found for this branch'); - core.setOutput('workflow_url', `https://github.com/${remoteOwner}/${remoteRepo}/actions/workflows/stable-merge-check.yml`); + core.setOutput('workflow_url', `https://github.com/${remoteOwner}/${remoteRepo}/actions/workflows/stable-merge-check.yml`); }
25-89: Add error recovery or timeout handling for remote API calls.The remote workflow status check can fail silently or hang if the API is slow or unresponsive. Consider adding a timeout or retry mechanism to ensure the workflow doesn't get stuck waiting for the remote repository.
You could add request timeouts or retry logic using GitHub's API client options, or add a step timeout at the job level:
check-remote-workflow-status: name: Check Remote Integration Tests runs-on: ubuntu-latest + timeout-minutes: 5 # Only run on PRs targeting stable branch if: github.event.pull_request.base.ref == 'stable'
96-114: Ensure workflow step always completes even on API failures.The "Set status check" step (lines 91–114) does not have error handling. If the
check-remotestep fails or outputs are malformed, this step may not execute or may produce unexpected behavior. Consider adding explicit error handling to ensure a status check is always set on the commit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/stable-branch-gate.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: unittest
- GitHub Check: functest
🔇 Additional comments (2)
.github/workflows/stable-branch-gate.yml (2)
117-119: Verifyprovide-instructionsjob runs on all relevant PR events.The job only runs when
github.event.action == 'opened'. This means contributors who push updates (synchronize) or reopen the PR won't see the instructions again. Consider whether this is intentional or if the job should run on other events (e.g.,synchronize,reopened) as well.
145-150: Critical: Incomplete script syntax.The
createCommentcall is missing its closing statement. Line 150 ends mid-function call, which will cause a syntax error when the workflow runs.await github.rest.issues.createComment({ owner: context.repo.owner, repo: context.repo.repo, issue_number: prNumber, body: comment - }); + }); + `Ensure the entire script block is closed with a backtick after the
});line.Likely an incorrect or invalid review comment.
| try { | ||
| const remoteOwner = 'opendatahub-io'; | ||
| const remoteRepo = 'data-science-pipelines'; | ||
|
|
||
| // Try to get workflow runs for the stable-merge-check workflow | ||
| const { data: workflows } = await github.rest.actions.listWorkflowRuns({ | ||
| owner: remoteOwner, | ||
| repo: remoteRepo, | ||
| workflow_id: 'stable-merge-check.yml', | ||
| branch: headRef, // Look for runs on the same branch name | ||
| per_page: 20 | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify cross-repo API access and workflow_id parameter.
The workflow queries the remote repository's workflow runs using the default GITHUB_TOKEN. However, there are two potential concerns:
-
Workflow ID format (line 48): The
workflow_idparameter is set to'stable-merge-check.yml'. Verify this is the correct format—GitHub API expects either the workflow file name or numeric ID. If the remote repository has branch protections or is private, this request may fail. -
Token permissions: The
GITHUB_TOKENgenerated in this repository may lack permissions to access workflow runs in the remoteopendatahub-io/data-science-pipelinesrepository, especially if that repository restricts API access or has organization policies.
Please verify:
- The remote repository's workflow file name matches
stable-merge-check.yml - The
GITHUB_TOKENhas sufficient permissions to query the remote repository's workflow runs (test with a manual workflow run or confirm the remote repo is public and API access is permitted)
🌐 Web query:
GitHub Actions API workflow_id parameter format and cross-repository workflow access permissions
💡 Result:
Short answer
- workflow_id accepts either the numeric workflow ID or the workflow file name (string, e.g. main.yml). [1]
- Permissions: the token you use must have rights on the target repository. The repo's GITHUB_TOKEN is an installation token limited to the repository where the workflow runs and its permissions are scoped; it can be used to create workflow_dispatch/repository_dispatch in the same repo but cannot be used to dispatch into a different repo. To trigger a workflow in another repository you must use a token with repo/Actions write permissions (for example a PAT or a GitHub App installation token with the appropriate repo Actions permissions). [2][3]
Sources
- GitHub REST API — Workflows (shows workflow_id may be the numeric ID or file name). [1]
- About GITHUB_TOKEN / token permissions (GITHUB_TOKEN is an installation token limited to the repository; behavior for dispatch events). [2]
- Guidance / marketplace action docs (dispatching to a remote repo requires a token with repo/write permissions such as a PAT or GitHub App token). [3]
🏁 Script executed:
#!/bin/bash
# Examine the full stable-branch-gate.yml workflow to see how github context is initialized
# and whether a custom token is being passed
cat .github/workflows/stable-branch-gate.ymlLength of output: 6511
GitHub token lacks permissions to access remote repository workflow runs—will cause runtime failure.
The workflow uses the default GITHUB_TOKEN (line 17: github-token: ${{ secrets.GITHUB_TOKEN }}) to query workflow runs in the remote opendatahub-io/data-science-pipelines repository (lines 44-51). However, the default GITHUB_TOKEN is scoped to the repository where the workflow runs and cannot access other repositories. The listWorkflowRuns() call will fail at runtime.
Required fix: Replace the default token with a token that has cross-repository access permissions:
- Use a Personal Access Token (PAT) with
repoandactions:readpermissions, OR - Use a GitHub App installation token with appropriate
Actionsread permissions
Store the token securely (as secrets.CROSS_REPO_TOKEN or similar) and update line 17 to: github-token: ${{ secrets.CROSS_REPO_TOKEN }}
Note: The workflow_id: 'stable-merge-check.yml' format (line 48) is correct; the API accepts both workflow file names and numeric IDs.
🤖 Prompt for AI Agents
.github/workflows/stable-branch-gate.yml lines 40-51: the workflow uses the
repository-scoped GITHUB_TOKEN to call github.rest.actions.listWorkflowRuns on
opendatahub-io/data-science-pipelines which will fail due to insufficient
cross-repo permissions; replace the default token with a secret that holds a
cross-repo-capable token (e.g., a PAT with repo + actions:read or a GitHub App
installation token) by adding that secret (e.g., secrets.CROSS_REPO_TOKEN) and
updating the workflow input that supplies github-token to use that secret
instead of GITHUB_TOKEN; ensure the secret is created and has the necessary
permissions before running the workflow.
| // Get PR information | ||
| const prNumber = context.payload.pull_request.number; | ||
| const headSha = context.payload.pull_request.head.sha; | ||
| const headRef = context.payload.pull_request.head.ref; |
There was a problem hiding this comment.
the default branch in dspo is main whereas in dsp it's master wouldn't this break?
There was a problem hiding this comment.
good catch, I will make that change
There was a problem hiding this comment.
sorry this will be stable, which is what we want I think
| runs-on: ubuntu-latest | ||
|
|
||
| # Only run on PRs targeting stable branch | ||
| if: github.event.pull_request.base.ref == 'stable' |
There was a problem hiding this comment.
don't we also want to confirm that the head ref is main? otherwise this workflow will fail right?
There was a problem hiding this comment.
For hot fixes or cherry picks, the head ref will not be main, so I think we should run this check for all prs to stable branch
| issues: write | ||
|
|
||
| jobs: | ||
| check-remote-workflow-status: |
There was a problem hiding this comment.
I might have missed it, but if the check upon PR open fails, can we retrigger it after having it opened?
There was a problem hiding this comment.
the check actually should not fail if the remote branch exists, I can add a polling wait for the remote workflow to either fail/pass though if the branch exists
There was a problem hiding this comment.
correct me if I'm wrong but, in this scenario:
- I create a PR from main -> stable in dspo
- this is triggered, the workflow then checks workflows in dsp that are also running for a similar pr that target dsp main
- if it doesn't find any runs, or if any runs failed, it fails the check
Suppose I re-run the DSP CI on the DSP pr from master -> stable, and I want to retrigger this check on the DSPO master, how would I do that?
Signed-off-by: Nelesh Singla <[email protected]>
|
[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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/stable-branch-gate.yml (1)
29-29: [DUPLICATE] GITHUB_TOKEN lacks cross-repository permissions—will cause runtime failure.This critical issue was flagged in the prior review and remains unresolved. Line 29 uses the default
secrets.GITHUB_TOKEN, which is scoped to the current repository only. The API call at line 49 (github.rest.actions.listWorkflowRuns()) targeting the remoteopendatahub-io/data-science-pipelinesrepository will fail at runtime with insufficient permissions.Required fix: Create a secret (e.g.,
secrets.CROSS_REPO_TOKEN) with a Personal Access Token (PAT) or GitHub App token that hasrepoandactions:readpermissions for the remote repository, then update line 29 to use that secret:github-token: ${{ secrets.CROSS_REPO_TOKEN }}Verify the secret is configured in the repository settings before the workflow runs.
Also applies to: 49-55
🧹 Nitpick comments (1)
.github/workflows/stable-branch-gate.yml (1)
22-22: [OPTIONAL] Consolidate duplicated branch conditions.Both the
check-remote-workflow-status(line 22) andprovide-instructions(line 123) jobs check the identical conditiongithub.event.pull_request.base.ref == 'stable'. Consider moving this check to the workflow-levelifor consolidating into a shared condition to reduce duplication:jobs: check-remote-workflow-status: if: github.event.pull_request.base.ref == 'stable' ... provide-instructions: if: github.event.pull_request.base.ref == 'stable' && github.event.action == 'opened' ...This is minor, but the
stablecheck is repeated unnecessarily.Also applies to: 123-123
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/stable-branch-gate.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: functest
- GitHub Check: unittest
🔇 Additional comments (1)
.github/workflows/stable-branch-gate.yml (1)
1-14: LGTM.The workflow trigger and permission scopes are appropriate for gating stable branch merges.
| per_page: 20 | ||
| }); | ||
|
|
||
| console.log(`Found ${workflows.total_count} workflow runs for branch ${headRef}`); |
There was a problem hiding this comment.
Fix misleading log message.
Line 57 logs headRef but the code actually queries remoteBranch (which may differ when headRef is 'main'). Update the log to reflect what was actually queried:
- console.log(`Found ${workflows.total_count} workflow runs for branch ${headRef}`);
+ console.log(`Found ${workflows.total_count} workflow runs for branch ${remoteBranch}`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log(`Found ${workflows.total_count} workflow runs for branch ${headRef}`); | |
| console.log(`Found ${workflows.total_count} workflow runs for branch ${remoteBranch}`); |
🤖 Prompt for AI Agents
.github/workflows/stable-branch-gate.yml around line 57: the log prints headRef
but the workflow query used remoteBranch, causing a misleading message when
headRef differs (e.g., 'main'). Change the log to reference the actual variable
used in the query (remoteBranch) and include both values if helpful (e.g.,
remoteBranch and headRef) so the message accurately reflects which branch was
queried.
| const { data: workflows } = await github.rest.actions.listWorkflowRuns({ | ||
| owner: remoteOwner, | ||
| repo: remoteRepo, | ||
| workflow_id: 'stable-merge-check.yml', | ||
| branch: remoteBranch, // Look for runs on the mapped branch name | ||
| per_page: 20 | ||
| }); |
There was a problem hiding this comment.
I think I'm a bit confused, so stable-merge-check.yml runs on a pull request to stable in DSP, but doesn't the above code search runs on the stable branch (i.e. if we ran this check post-merge on stable)? would that work?
2 participants
The issue resolved by this Pull Request:
Resolves #<issue_number>
Description of your changes:
Testing instructions
Checklist
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.