From e710622d4c03c8a4549a93f70c993f22d0676910 Mon Sep 17 00:00:00 2001 From: Charlie Doern Date: Thu, 18 Dec 2025 15:24:09 -0500 Subject: [PATCH] fix: run all clients on stainless SDK, fix workflow, properly commit recordings (#4410) # What does this PR do? Various fixes to integration test recording + stainless calling of integration tests: 1. only the library client was being run, they all should be 2. the git check grabs diffs like: M tests/integration/client-typescript/package-lock.json M tests/integration/client-typescript/package.json it should not additionally: Fixes rebase conflicts when stainless workflow runs integration tests with record-if-missing mode on PRs. Previously, the workflow would: 1. Commit all files in tests/integration/ (including non-recordings) 2. Try to rebase and push to 'main' instead of the PR branch 3. Fail with merge conflicts on PR-specific changes Changes: - Add pr_head_ref and is_fork_pr parameters flowing through workflow chain - Use target-branch input instead of github.ref_name in recording commits - Detect and handle fork PRs by skipping push and uploading recordings as artifacts - Add 7-day artifact retention for fork PR recordings - Support both workflow_call and direct pull_request trigger contexts For same-repo PRs: recordings now commit/push to the PR branch correctly For fork PRs: recordings upload as downloadable artifacts with instructions you can see a failing workflow: https://github.com/llamastack/llama-stack/actions/runs/20347927155/job/58465906136 with the rebase issues. --------- Signed-off-by: Charlie Doern --- .../actions/run-and-record-tests/action.yml | 54 +++++++++++++++---- .github/workflows/integration-tests.yml | 11 ++++ .github/workflows/stainless-builds.yml | 6 +++ tests/integration/ci_matrix.json | 2 +- 4 files changed, 62 insertions(+), 11 deletions(-) diff --git a/.github/actions/run-and-record-tests/action.yml b/.github/actions/run-and-record-tests/action.yml index a5520a571..8d6c361b4 100644 --- a/.github/actions/run-and-record-tests/action.yml +++ b/.github/actions/run-and-record-tests/action.yml @@ -24,6 +24,14 @@ inputs: description: 'Regex pattern to pass to pytest -k' required: false default: '' + target-branch: + description: 'Target branch for recording commits (for PRs, use the PR head branch)' + required: false + default: '' + is-fork-pr: + description: 'Whether this is a fork PR (recordings cannot be pushed to forks)' + required: false + default: 'false' runs: using: 'composite' @@ -66,23 +74,49 @@ runs: shell: bash run: | echo "Checking for recording changes" - git status --porcelain tests/integration/ + git status --porcelain tests/integration/recordings/ tests/integration/*/recordings/ - if [[ -n $(git status --porcelain tests/integration/) ]]; then - echo "New recordings detected, committing and pushing" - git add tests/integration/ + if [[ -n $(git status --porcelain tests/integration/recordings/ tests/integration/*/recordings/) ]]; then + echo "New recordings detected" - git commit -m "Recordings update from CI (setup: ${{ inputs.setup }}, suite: ${{ inputs.suite }})" + # Determine target branch: use target-branch input if provided, otherwise use current branch + TARGET_BRANCH="${{ inputs.target-branch }}" + if [ -z "$TARGET_BRANCH" ]; then + TARGET_BRANCH="${{ github.ref_name }}" + fi + echo "Target branch: $TARGET_BRANCH" - git fetch origin ${{ github.ref_name }} - git rebase origin/${{ github.ref_name }} - echo "Rebased successfully" - git push origin HEAD:${{ github.ref_name }} - echo "Pushed successfully" + # Check if this is a fork PR + if [ "${{ inputs.is-fork-pr }}" = "true" ]; then + echo "::warning::This is a fork PR. Recordings were updated locally but cannot be pushed to the fork." + echo "::warning::Please download the workflow artifacts and commit the recordings manually." + else + echo "Committing and pushing recordings to branch: $TARGET_BRANCH" + git add tests/integration/recordings/ tests/integration/*/recordings/ + + git commit -m "Recordings update from CI (setup: ${{ inputs.setup }}, suite: ${{ inputs.suite }})" + + git fetch origin "$TARGET_BRANCH" + git rebase "origin/$TARGET_BRANCH" + echo "Rebased successfully" + git push origin "HEAD:$TARGET_BRANCH" + echo "Pushed successfully to $TARGET_BRANCH" + fi else echo "No recording changes" fi + - name: Upload recordings (for fork PRs) + if: ${{ inputs.is-fork-pr == 'true' && (inputs.inference-mode == 'record' || inputs.inference-mode == 'record-if-missing') }} + uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 + with: + name: recordings-${{ github.run_id }}-${{ github.run_attempt || '1' }}-${{ strategy.job-index || github.job }} + path: | + tests/integration/recordings/ + tests/integration/*/recordings/ + retention-days: 7 + if-no-files-found: ignore + - name: Write docker logs to file if: ${{ always() }} shell: bash diff --git a/.github/workflows/integration-tests.yml b/.github/workflows/integration-tests.yml index 2eb64382f..c202deafe 100644 --- a/.github/workflows/integration-tests.yml +++ b/.github/workflows/integration-tests.yml @@ -52,6 +52,15 @@ on: required: false type: string description: 'The SHA of the pull request head to checkout' + pr_head_ref: + required: false + type: string + description: 'The branch name of the pull request head (for recording commits)' + is_fork_pr: + required: false + type: boolean + default: false + description: 'Whether this is a fork PR (cannot push recordings to forks)' test-all-client-versions: required: false type: boolean @@ -150,3 +159,5 @@ jobs: setup: ${{ matrix.config.setup }} inference-mode: ${{ matrix.config.inference_mode || 'replay' }} suite: ${{ matrix.config.suite }} + target-branch: ${{ inputs.pr_head_ref || '' }} + is-fork-pr: ${{ inputs.is_fork_pr && 'true' || (github.event.pull_request.head.repo.full_name != github.repository && 'true' || 'false') }} diff --git a/.github/workflows/stainless-builds.yml b/.github/workflows/stainless-builds.yml index e25b2103e..cec840512 100644 --- a/.github/workflows/stainless-builds.yml +++ b/.github/workflows/stainless-builds.yml @@ -66,6 +66,7 @@ jobs: pr_base_sha: ${{ steps.compute.outputs.pr_base_sha }} pr_base_ref: ${{ steps.compute.outputs.pr_base_ref }} pr_title: ${{ steps.compute.outputs.pr_title }} + is_fork_pr: ${{ steps.compute.outputs.is_fork_pr }} steps: - name: Fetch PR details for workflow_dispatch if: github.event_name == 'workflow_dispatch' @@ -111,10 +112,12 @@ jobs: fi PREVIEW_BRANCH="preview/${FORK_OWNER}/${BRANCH_NAME}" BASE_BRANCH="preview/base/${FORK_OWNER}/${BRANCH_NAME}" + IS_FORK_PR="true" else # Same-repo PR PREVIEW_BRANCH="preview/${BRANCH_NAME}" BASE_BRANCH="preview/base/${BRANCH_NAME}" + IS_FORK_PR="false" fi echo "preview_branch=${PREVIEW_BRANCH}" >> $GITHUB_OUTPUT @@ -126,6 +129,7 @@ jobs: echo "pr_base_sha=${BASE_SHA}" >> $GITHUB_OUTPUT echo "pr_base_ref=${BASE_REF}" >> $GITHUB_OUTPUT echo "pr_title=${PR_TITLE}" >> $GITHUB_OUTPUT + echo "is_fork_pr=${IS_FORK_PR}" >> $GITHUB_OUTPUT preview: needs: compute-branch @@ -182,6 +186,8 @@ jobs: matrix_key: 'stainless' test-all-client-versions: false pr_head_sha: ${{ needs.compute-branch.outputs.pr_head_sha }} + pr_head_ref: ${{ needs.compute-branch.outputs.pr_head_ref }} + is_fork_pr: ${{ needs.compute-branch.outputs.is_fork_pr == 'true' }} merge: needs: compute-branch diff --git a/tests/integration/ci_matrix.json b/tests/integration/ci_matrix.json index 623acfbdf..d8f7b1470 100644 --- a/tests/integration/ci_matrix.json +++ b/tests/integration/ci_matrix.json @@ -7,7 +7,7 @@ {"suite": "base-vllm-subset", "setup": "vllm"} ], "stainless": [ - {"suite": "base", "setup": "ollama", "allowed_clients": ["library"], "inference_mode": "record-if-missing"} + {"suite": "base", "setup": "ollama", "inference_mode": "record-if-missing"} ], "schedules": { "1 0 * * 0": [