Skip to content

fix(ci): enable auto-approve for infrastructure-only PRs#222

Open
eric-wang-1990 wants to merge 2 commits intomainfrom
fix/auto-approve-pr-infrastructure-changes
Open

fix(ci): enable auto-approve for infrastructure-only PRs#222
eric-wang-1990 wants to merge 2 commits intomainfrom
fix/auto-approve-pr-infrastructure-changes

Conversation

@eric-wang-1990
Copy link
Collaborator

Problem

The integration test workflow had a chicken-and-egg problem:

  1. Branch protection requires "Integration Tests" check to pass
  2. The workflow had auto-approve logic for infrastructure-only changes
  3. BUT the auto-approve only worked for merge queue, not for PRs
  4. So infrastructure-only PRs still needed manual integration-test labels

This was demonstrated by PR #209 - a PR that only removed test-infrastructure/ and .github/workflows/proxy-tests.yml still needed a manual label to create the required check.

Solution

Modified the auto-approve-no-relevant-changes job to work for both PR and merge queue events:

Changes

  1. Unified path checking (check-paths job):

    • Now runs for both pull_request and merge_group events
    • Renamed from check-merge-queue-paths for clarity
    • Added more driver paths: go/, rust/src/, rust/examples/
  2. Auto-approve for PRs (auto-approve-no-relevant-changes):

    • Extended condition to handle both event types
    • Uses dynamic SHA detection based on event type
    • Adds helpful comment on PRs explaining why tests were skipped
  3. Updated triggers:

    • Added opened event so checks run when PR is first created

Behavior

Before:

PR with only infrastructure changes → blocked → need manual label → tests skip anyway

After:

PR with only infrastructure changes → auto-approved immediately → no manual label needed
PR with driver code changes → need manual label (security) → tests run

Paths that trigger integration tests

  • .github/workflows/trigger-integration-tests.yml
  • ci/scripts/
  • csharp/src/ & csharp/test/
  • go/
  • rust/src/ & rust/examples/

Changes to other paths (docs, configs, removed test-infrastructure, etc.) automatically get approved.

Testing

This PR itself modifies only .github/workflows/trigger-integration-tests.yml, so it should auto-approve with the new logic! 🎯

🤖 Generated with Claude Code

Modifies the integration test workflow to automatically approve PRs that only change infrastructure/documentation files, eliminating the need for manual `integration-test` labels on such PRs.

## Changes

1. **Unified path checking** (`check-paths` job):
   - Now runs for both `pull_request` and `merge_group` events
   - Detects whether driver code was changed
   - Added more driver paths: `go/`, `rust/src/`, `rust/examples/`

2. **Auto-approve for PRs** (`auto-approve-no-relevant-changes` job):
   - Extended to work for both PR and merge queue events
   - Automatically creates passing "Integration Tests" check when only infrastructure files change
   - Adds explanatory comment on PRs for visibility

3. **Updated triggers**:
   - Added `opened` event so auto-approve runs when PR is first created

## Behavior

**Before**: Infrastructure-only PRs (like removing test-infrastructure) required manual `integration-test` label to create the required check, even though tests should be skipped.

**After**: Infrastructure-only PRs automatically get a passing "Integration Tests" check without manual intervention. Tests only run when driver code changes.

## Paths that trigger tests
- `.github/workflows/trigger-integration-tests.yml`
- `ci/scripts/`
- `csharp/src/` & `csharp/test/`
- `go/`
- `rust/src/` & `rust/examples/`

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@eric-wang-1990
Copy link
Collaborator Author

Addressed review feedback:

  1. ✅ Removed go/, rust/src/, and rust/examples/ from the paths list - now only tracking C# driver paths (csharp/src/, csharp/test/)

  2. ✅ Simplified the PR comment - removed the detailed path listing

Changes made per your review comments. This workflow is specifically for the Databricks ADBC C# driver.

Comment on lines +179 to +181
"go/"
"rust/src/"
"rust/examples/"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Remove these 3

Comment on lines +358 to +360
'- `go/`\n' +
'- `rust/src/`\n' +
'- `rust/examples/`\n\n' +
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

remove these

'- `go/`\n' +
'- `rust/src/`\n' +
'- `rust/examples/`\n\n' +
'Your changes only affect infrastructure/documentation, so tests are skipped.'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Your changes do not affect Databricks ADBC csharp driver

- Remove go/, rust/src/, rust/examples/ from paths list (C# driver only)
- Simplify auto-approve PR comment (remove detailed path listing)

Per review comments: This workflow is specifically for the Databricks ADBC C# driver.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@eric-wang-1990
Copy link
Collaborator Author

Changes have been committed and pushed. Please re-review when you have a chance. 🙏

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR aims to fix a chicken-and-egg problem where infrastructure-only PRs were blocked waiting for the "Integration Tests" check, even though they would be auto-approved. The solution extends the auto-approve logic from merge queue events to also work for pull request events.

Changes:

  • Added opened event trigger so checks run when a PR is first created
  • Renamed check-merge-queue-paths to check-paths and extended it to run for both PR and merge_group events
  • Modified auto-approve-no-relevant-changes to work for both event types with dynamic SHA detection
  • Added PR comment functionality to explain why tests were skipped for infrastructure-only changes

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

".github/workflows/trigger-integration-tests.yml"
"ci/scripts/"
"csharp/src/"
"csharp/test/"
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

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

The PR description states that "go/" and "rust/src/" and "rust/examples/" were added to the paths that trigger integration tests, but the PATHS array only includes ".github/workflows/trigger-integration-tests.yml", "ci/scripts/", "csharp/src/", and "csharp/test/".

The go/ and rust/ directories exist in the repository and contain driver code that should trigger integration tests. These paths need to be added to the PATHS array to match the documented behavior and ensure that changes to Go and Rust driver code trigger integration tests appropriately.

Suggested change
"csharp/test/"
"csharp/test/"
"go/"
"rust/src/"
"rust/examples/"

Copilot uses AI. Check for mistakes.
Comment on lines +343 to +351
// Add a comment on PR for visibility
if (isPR) {
await github.rest.issues.createComment({
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: context.issue.number,
body: '✅ **Integration tests auto-approved**\n\n' +
'No driver code changes detected. Your changes only affect infrastructure/documentation, so tests are skipped.'
});
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

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

The auto-approve comment on line 345-351 could be created multiple times if the workflow is re-run for the same PR. This could spam the PR with duplicate "Integration tests auto-approved" comments.

Consider adding logic to check if a comment with this message already exists before creating a new one. You can search for existing comments and either skip creation or update the existing comment instead of creating duplicates.

Suggested change
// Add a comment on PR for visibility
if (isPR) {
await github.rest.issues.createComment({
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: context.issue.number,
body: '✅ **Integration tests auto-approved**\n\n' +
'No driver code changes detected. Your changes only affect infrastructure/documentation, so tests are skipped.'
});
// Add a comment on PR for visibility (avoid duplicates)
if (isPR) {
const commentBody =
'✅ **Integration tests auto-approved**\n\n' +
'No driver code changes detected. Your changes only affect infrastructure/documentation, so tests are skipped.';
// Check if an identical comment already exists to avoid spamming the PR
const { data: existingComments } = await github.rest.issues.listComments({
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: context.issue.number,
per_page: 100,
});
const alreadyCommented = existingComments.some(
(comment) => comment.body === commentBody
);
if (!alreadyCommented) {
await github.rest.issues.createComment({
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: context.issue.number,
body: commentBody,
});
}

Copilot uses AI. Check for mistakes.
Comment on lines 174 to 179
PATHS=(
".github/workflows/trigger-integration-tests.yml"
"ci/scripts/"
"csharp/src/"
"csharp/test/"
)
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

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

The PR description claims "Added more driver paths: go/, rust/src/, rust/examples/" in the "Unified path checking" changes section, but these paths are not actually added to the PATHS array in the code. This is a significant discrepancy between the PR description and the actual implementation.

Either the paths need to be added to match the description, or the description needs to be updated to reflect that these paths were not added in this PR.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants