Skip to content

fix: go to gh runner for broken test#4125

Closed
Flo4604 wants to merge 2 commits intomainfrom
fix/broken-test
Closed

fix: go to gh runner for broken test#4125
Flo4604 wants to merge 2 commits intomainfrom
fix/broken-test

Conversation

@Flo4604
Copy link
Member

@Flo4604 Flo4604 commented Oct 20, 2025

What does this PR do?

Switches back to normal GH runner for the test that keeps failing, also only runs docs linting when we change docs....

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Chore (refactoring code, technical debt, workflow improvements)
  • Enhancement (small improvements)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How should this be tested?

Test go green?

Checklist

Required

  • Filled out the "How to test" section in this PR
  • Read Contributing Guide
  • Self-reviewed my own code
  • Commented on my code in hard-to-understand areas
  • Ran pnpm build
  • Ran pnpm fmt
  • Checked for warnings, there are none
  • Removed all console.logs
  • Merged the latest changes from main onto my branch with git pull origin main
  • My changes don't cause any responsiveness issues

Appreciated

  • If a UI change was made: Added a screen recording or screenshots to this PR
  • Updated the Unkey Docs if changes were necessary

@changeset-bot
Copy link

changeset-bot bot commented Oct 20, 2025

⚠️ No Changeset found

Latest commit: 06a5d94

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link

vercel bot commented Oct 20, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
engineering Ready Ready Preview Comment Oct 21, 2025 5:23pm
1 Skipped Deployment
Project Deployment Preview Comments Updated (UTC)
dashboard Ignored Ignored Preview Oct 21, 2025 5:23pm

@github-actions
Copy link
Contributor

github-actions bot commented Oct 20, 2025

Thank you for following the naming conventions for pull request titles! 🙏

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 20, 2025

📝 Walkthrough

Walkthrough

Adds a change-detection job to the autofix CI workflow to gate downstream tasks, and standardizes several workflows to use ubuntu-latest runners and the official Docker Buildx action in place of custom Blacksmith runner/actions.

Changes

Cohort / File(s) Summary
Autofix: change detection gating
.github/workflows/autofix.ci.yaml
Added detect_changes job calling a reusable workflow; updated lint_docs to needs: [detect_changes] and added an if condition requiring needs.detect_changes.result == 'success' and docs output
Test workflows: runner and Buildx action updates
.github/workflows/job_test_api_local.yaml, .github/workflows/job_test_go_api_local.yaml
Replaced specific Blacksmith runners with ubuntu-latest (original values retained as comments); swapped useblacksmith/setup-docker-builder for docker/setup-buildx-action and commented previous action lines
Release workflow: runner change
.github/workflows/release.yaml
Changed goreleaser job runs-on from blacksmith-4vcpu-ubuntu-2404 to ubuntu-latest and added commented original runs-on line

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant PR as Pull Request
    participant GH as GitHub Actions
    participant Detect as detect_changes (reusable)
    participant Lint as lint_docs job

    PR->>GH: trigger autofix.ci workflow
    GH->>Detect: run detect_changes (reusable workflow)
    alt detect_changes success and docs changed
        Detect-->>GH: result: success, docs_output: true
        GH->>Lint: start lint_docs (needs satisfied + if true)
        Lint-->>GH: lint result
    else detect_changes failure or no docs
        Detect-->>GH: result: failure or docs_output:false
        GH-->>Lint: skip lint_docs (condition not met)
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The PR title "fix: go to gh runner for broken test" is directly related to the main changes in the changeset. The pull request primarily involves switching CI runners from custom Blacksmith VMs (blacksmith-8vcpu-ubuntu-2404, blacksmith-16vcpu-ubuntu-2404, blacksmith-4vcpu-ubuntu-2404) to ubuntu-latest across multiple workflow files (.github/workflows/autofix.ci.yaml, job_test_api_local.yaml, job_test_go_api_local.yaml, and release.yaml). The title clearly captures this main infrastructure change. While the PR also includes updates to the Docker Buildx action and a new docs-change detection mechanism, the primary focus on migrating to GitHub's standard runners is well-represented in the title, which is appropriately concise and specific.
Description Check ✅ Passed The PR description follows the required template structure and includes all major sections: "What does this PR do?" (with a clear summary of the changes), "Type of change" (marked as Bug fix), "How should this be tested?" (though minimal), and the "Checklist" section. The description is mostly complete and directly addresses the key changes—switching to GitHub runners and restricting docs linting to documentation changes. While the testing instructions are brief ("Test go green?") and all checklist items remain unchecked, the description is sufficiently detailed for the nature of the changes (CI/workflow infrastructure updates) and aligns with the template requirements.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/broken-test

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 632edc7 and 06a5d94.

📒 Files selected for processing (2)
  • .github/workflows/job_test_go_api_local.yaml (1 hunks)
  • .github/workflows/release.yaml (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T19:52:42.113Z
Learnt from: imeyer
PR: unkeyed/unkey#3765
File: .github/workflows/job_detect_changes.yaml:43-43
Timestamp: 2025-08-08T19:52:42.113Z
Learning: In the unkeyed/unkey repository, the workflows `.github/workflows/job_test_api_local.yaml` and `.github/workflows/job_test_go_api_local.yaml` should keep their Blacksmith self-hosted runners (blacksmith-4vcpu-ubuntu-2404 and blacksmith-8vcpu-ubuntu-2404 respectively) as these are performance-critical test jobs that require dedicated resources.

Applied to files:

  • .github/workflows/job_test_go_api_local.yaml
⏰ 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). (3)
  • GitHub Check: Test Go API Local / Test
  • GitHub Check: Test API / API Test Local
  • GitHub Check: Build / Build
🔇 Additional comments (3)
.github/workflows/job_test_go_api_local.yaml (2)

18-19: Verify Docker action change for compatibility.

The switch from useblacksmith/setup-docker-builder to docker/setup-buildx-action is a significant change. Ensure:

  • The official Docker Buildx action provides equivalent functionality for the integration tests (especially Docker Compose setup in lines 14-16).
  • No breaking changes in how Docker BuildKit environment variables (lines 28-29) are handled.
  • The action version pinning is appropriate (v3.11.1).

Consider running a full test cycle to confirm the new action works correctly with your test environment.


9-10: Disregard the review comment; the learning's interpretation of PR #3765 does not match its stated intent.

The retrieved learning claims PR #3765 documented that these workflows "should keep" Blacksmith runners. However, PR #3765's actual commit message states "fix(ci): change less important jobs to gha runners," suggesting the change was intentional and these workflows were classified as "less important."

A more recent commit (06a5d94e, today) deliberately standardizes all workflows to ubuntu-latest with the message "just use normal runners." Both job_test_go_api_local.yaml and job_test_api_local.yaml now consistently use ubuntu-latest, reflecting this architecture decision. The current change aligns with this uniform strategy, not a deviation from documented best practices.

Likely an incorrect or invalid review comment.

.github/workflows/release.yaml (1)

13-14: Verify release workflow performance with ubuntu-latest runner.

The goreleaser job is switching from blacksmith-4vcpu-ubuntu-2404 to ubuntu-latest. This is lower-risk than test workflows (runs less frequently), but binary compilation/release builds can be resource-intensive.

Verify that:

  • GoReleaser builds complete successfully on ubuntu-latest without timeout issues.
  • Build times are acceptable (note: ubuntu-latest is typically slower than dedicated 4vcpu runners).
  • No platform-specific build failures occur.

If build times become problematic, consider reverting to a self-hosted runner for this job specifically.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@vercel vercel bot temporarily deployed to Preview – dashboard October 20, 2025 19:34 Inactive
@graphite-app
Copy link

graphite-app bot commented Oct 20, 2025

Graphite Automations

"Notify author when CI fails" took an action on this PR • (10/20/25)

1 teammate was notified to this PR based on Andreas Thomas's automation.

"Post a GIF when PR approved" took an action on this PR • (10/21/25)

1 gif was posted to this PR based on Andreas Thomas's automation.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c764837 and 632edc7.

📒 Files selected for processing (2)
  • .github/workflows/autofix.ci.yaml (2 hunks)
  • .github/workflows/job_test_api_local.yaml (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T19:52:42.113Z
Learnt from: imeyer
PR: unkeyed/unkey#3765
File: .github/workflows/job_detect_changes.yaml:43-43
Timestamp: 2025-08-08T19:52:42.113Z
Learning: In the unkeyed/unkey repository, the workflows `.github/workflows/job_test_api_local.yaml` and `.github/workflows/job_test_go_api_local.yaml` should keep their Blacksmith self-hosted runners (blacksmith-4vcpu-ubuntu-2404 and blacksmith-8vcpu-ubuntu-2404 respectively) as these are performance-critical test jobs that require dedicated resources.

Applied to files:

  • .github/workflows/job_test_api_local.yaml
⏰ 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). (3)
  • GitHub Check: Test API / API Test Local
  • GitHub Check: Test Go API Local / Test
  • GitHub Check: Build / Build
🔇 Additional comments (3)
.github/workflows/job_test_api_local.yaml (1)

15-16: Docker Buildx action update is reasonable.

The change from useblacksmith/setup-docker-builder to the official docker/setup-buildx-action@v3.11.1 is a sensible modernization that aligns with standard CI practices. The prior action reference is retained as a comment for reference.

.github/workflows/autofix.ci.yaml (2)

42-43: Compound if condition mixing needs and github.event context — verify logic.

The lint_docs if condition combines job outputs from needs.detect_changes with GitHub event context. While this pattern is valid, ensure the logic handles these scenarios correctly:

  • If detect_changes is skipped (draft PR or non-pull_request event), the condition should still evaluate correctly
  • If detect_changes fails, lint_docs will be skipped (is this intended?)

The current condition will cause lint_docs to be skipped if detect_changes.result is not 'success' (e.g., if it's 'skipped' or 'failure'). Verify this is the intended behavior.

Can you confirm that this gating logic aligns with the expected CI behavior?


9-11: All verification points confirmed—the workflow is properly configured.

The job_detect_changes.yaml workflow exists and is correctly implemented:

  • ✓ Defines a docs output with proper chaining (jobs.build.outputs.docs ← steps.changes.outputs.docs)
  • ✓ Includes path patterns for detecting docs changes (apps/docs/, apps/engineering/, *.md)
  • ✓ The lint_docs job gating logic is sound:
    • Checks needs.detect_changes.result == 'success' (handles workflow failure)
    • Checks needs.detect_changes.outputs.docs == 'true' (handles missing output)
    • Conditional logic properly handles draft PRs and non-PR events

No issues found.

@Flo4604 Flo4604 enabled auto-merge October 20, 2025 19:53
@imeyer imeyer self-requested a review October 21, 2025 13:34
Copy link
Contributor

@imeyer imeyer left a comment

Choose a reason for hiding this comment

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

:shipit:

@graphite-app
Copy link

graphite-app bot commented Oct 21, 2025

Mike Myers Thumbs Up GIF (Added via Giphy)

@Flo4604 Flo4604 added this pull request to the merge queue Oct 21, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 21, 2025
@Flo4604 Flo4604 added this pull request to the merge queue Oct 21, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 21, 2025
@vercel vercel bot temporarily deployed to Preview – engineering October 21, 2025 17:23 Inactive
@Flo4604 Flo4604 added this pull request to the merge queue Oct 21, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 21, 2025
@Flo4604 Flo4604 added this pull request to the merge queue Oct 21, 2025
github-merge-queue bot pushed a commit that referenced this pull request Oct 21, 2025
* fix: go to gh runner for broken test

* just use normal runners
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 21, 2025
@Flo4604 Flo4604 added this pull request to the merge queue Oct 21, 2025
github-merge-queue bot pushed a commit that referenced this pull request Oct 21, 2025
* fix: go to gh runner for broken test

* just use normal runners
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 21, 2025
@Flo4604 Flo4604 added this pull request to the merge queue Oct 21, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 21, 2025
@Flo4604 Flo4604 added this pull request to the merge queue Oct 21, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 21, 2025
@Flo4604 Flo4604 added this pull request to the merge queue Oct 22, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 22, 2025
@Flo4604
Copy link
Member Author

Flo4604 commented Oct 22, 2025

superseeded by #4138

@Flo4604 Flo4604 closed this Oct 22, 2025
@Flo4604 Flo4604 deleted the fix/broken-test branch November 4, 2025 18:20
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.

3 participants