test: WORKFLOW TEST - DO NOT MERGE (delete after ENG-2146) Docs only change#4198
test: WORKFLOW TEST - DO NOT MERGE (delete after ENG-2146) Docs only change#4198
Conversation
mcstepp
requested review from
Flo4604,
MichaelUnkey,
chronark,
ogzhanolguncu and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughWorkflow configuration updates that introduce dashboard-specific testing in CI/CD pipelines. Changes refine path detection patterns for dashboard and Go code, add a new dashboard test workflow job, integrate it into the PR workflow with conditional triggering, and update existing job conditions for logical consistency. Changes
Sequence Diagram(s)sequenceDiagram
actor Workflow as PR Workflow
participant Detect as detect_changes Job
participant Dashboard as test_dashboard Job
Workflow->>Detect: Trigger
Detect->>Detect: Analyze changed paths<br/>(dashboard, go, etc.)
Detect-->>Workflow: Output change flags
rect rgba(100, 150, 200, 0.2)
Note over Workflow,Dashboard: Conditional trigger on changes
Workflow->>Dashboard: Trigger if<br/>dashboard == 'true'
end
Dashboard->>Dashboard: Checkout
Dashboard->>Dashboard: Setup Node
Dashboard->>Dashboard: Lint @unkey/dashboard
Dashboard->>Dashboard: Test @unkey/dashboard (CI=1)
Dashboard->>Dashboard: Build @unkey/dashboard (CI=1)
Dashboard-->>Workflow: Results
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✨ 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 |
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
apps/dashboard/README.md (1)
47-47: Remove placeholder test content before merging.Line 47 adds a placeholder "# Test" heading with no accompanying documentation. Since the PR description explicitly states "delete after ENG-2146," ensure this is removed before the PR is ready for production merge.
.github/workflows/job_detect_changes.yaml (1)
76-95: Verify expanded dashboard dependencies are accurate.The dashboard path filter has been significantly expanded from 2 entries to 11+ internal packages and 2 package entries. Verify that all added dependencies (billing, clickhouse, db, encryption, events, hash, id, keys, proto, schema, ui, packages/error, packages/rbac) are actual direct or indirect dependencies of @unkey/dashboard that should trigger dashboard CI jobs.
This expansion will cause the
dashboardoutput to trigger more frequently, potentially increasing CI/CD load.Consider documenting in a comment why each internal/packages path is a dashboard dependency, to reduce future maintenance burden and clarify intent.
.github/workflows/job_test_dashboard.yaml (2)
10-25: Add database initialization step before test and build.The workflow runs lint, test, and build steps but does not initialize or migrate the database. However, lines 26-29 define database connection variables (
DATABASE_HOST,DATABASE_USERNAME,DATABASE_PASSWORD,DATABASE_NAME), suggesting tests/build may require database access.Clarify the database setup approach:
- Are tests/build expected to connect to a real or mock database?
- Should this workflow include a database service container (e.g., Docker Compose, testcontainers, or local service)?
- If no actual database is needed, consider removing or externalizing these environment variables.
Without proper setup, the test or build step may fail with connection errors.
26-39: Review and document environment variable requirements.Lines 26-39 define environment variables with placeholder/test values. Clarify:
- DATABASE_HOST: localhost:8080 — Does the dashboard test suite actually connect to a local database in the CI runner? If yes, how is it initialized?
- Placeholder values ("not-empty", "client_", "sk_test_") — Document why these are sufficient for CI and whether they represent mocked or stubbed services.
- WORKOS_COOKIE_PASSWORD — Consider whether sensitive-looking values belong in the workflow file or should be sourced from GitHub secrets.
If any of these environment variables are not actually used by the
@unkey/dashboardpackage, remove them to reduce noise and clarify the workflow's intent..github/workflows/pr.yaml (1)
16-16: Consider extracting repeated conditional logic to reduce maintenance burden.The draft/event-name check pattern is repeated verbatim across multiple job conditions:
((github.event_name == 'pull_request' && github.event.pull_request.draft == false) || github.event_name != 'pull_request')This appears in
detect_changes(line 16),test_packages(line 20),build(line 25),test_api(line 30),test_go_api_local(line 35), and nowtest_dashboard(line 40). Extracting this to a job-level default or workflow variable would improve readability and reduce the risk of typos.This is a nice-to-have improvement for future refactoring, not a blocker.
Also applies to: 25-25, 30-30, 40-40
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/job_detect_changes.yaml(2 hunks).github/workflows/job_test_dashboard.yaml(1 hunks).github/workflows/pr.yaml(1 hunks)apps/dashboard/README.md(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: imeyer
PR: unkeyed/unkey#3755
File: .github/workflows/runbook-freshness-check.yaml:157-173
Timestamp: 2025-08-08T14:59:52.283Z
Learning: Repo unkeyed/unkey: When a CI/workflow fix is deferred, imeyer prefers a thorough GitHub issue be opened with sections (Summary, Impact, Where, Repro, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References) and assigned to imeyer, including backlinks to the originating PR and comment.
Learnt from: imeyer
PR: unkeyed/unkey#3755
File: .github/actions/setup-node/action.yaml:0-0
Timestamp: 2025-08-08T15:10:46.436Z
Learning: For repo unkeyed/unkey and PR review workflows: When imeyer comments "issue" on a thread, automatically create a thorough GitHub issue (sections: Summary, Impact, Where, Repro/Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References), include backlinks to the PR and the specific comment, and assign the issue to imeyer.
Learnt from: imeyer
PR: unkeyed/unkey#3755
File: .github/actions/setup-node/action.yaml:0-0
Timestamp: 2025-08-08T15:10:46.436Z
Learning: Repo: unkeyed/unkey — Preference: If imeyer comments “issue” on a PR thread, automatically open a thorough GitHub issue (Summary, Impact, Where, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References), include backlinks to the PR and comment, and assign to imeyer.
📚 Learning: 2025-09-18T17:28:24.803Z
Learnt from: Flo4604
PR: unkeyed/unkey#3994
File: go/apps/gw/server/middleware_metrics.go:13-14
Timestamp: 2025-09-18T17:28:24.803Z
Learning: The apps/agent package is a separate Go module that's not used by or integrated with the main gateway/API codebase. Interface inconsistencies between agent and main codebase components are not a concern since they operate independently.
Applied to files:
.github/workflows/job_detect_changes.yaml
📚 Learning: 2025-01-03T14:22:34.611Z
Learnt from: perkinsjr
PR: unkeyed/unkey#2787
File: apps/dashboard/middleware.ts:86-86
Timestamp: 2025-01-03T14:22:34.611Z
Learning: The user clarified that the `apps/dashboard` and `apps/www` are separate applications hosted on different subdomains.
Applied to files:
.github/workflows/job_detect_changes.yaml
📚 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_dashboard.yaml.github/workflows/pr.yaml
🪛 actionlint (1.7.8)
.github/workflows/job_test_dashboard.yaml
9-9: label "depot-ubuntu-24.04-4" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-11-arm", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-26-xlarge", "macos-26", "macos-15-intel", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
⏰ 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: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
.github/workflows/job_test_dashboard.yaml (1)
9-9: Resolve unknown runner label with actionlint configuration.Static analysis flags
depot-ubuntu-24.04-4as an unknown runner label. If this is a custom self-hosted runner or third-party CI provider runner, configure it in.github/actionlint.yamlto suppress the warning and document the runner for future developers.Reference: The same runner is used in
.github/workflows/job_detect_changes.yamlline 43.Update
.github/actionlint.yamlto include this runner label:runners: [depot-ubuntu-24.04-4].github/workflows/pr.yaml (2)
38-42: Dashboard test job integration looks good.The new
test_dashboardjob is properly integrated with:
- Correct dependency on
[detect_changes]for change detection outputs- Appropriate conditional trigger: only runs when dashboard changes are detected
- Consistent naming and structure with existing test jobs
- Proper wiring to
./.github/workflows/job_test_dashboard.yamlThis should trigger reliably whenever the dashboard or its dependencies change.
25-25: Verify updated build and test_api conditions are intentional.The conditional for both
build(line 25) andtest_api(line 30) now include an additional check:(needs.detect_changes.outputs.api == 'true' || needs.detect_changes.outputs.packages == 'true').This means these jobs will run when either API or packages change, which is reasonable since package changes can affect the API. However, confirm this was the intended change and not a side effect of the broader refactoring. If the packages output did not previously trigger these jobs, this could increase CI load.
Also applies to: 30-30
.github/workflows/job_detect_changes.yaml (1)
107-108: I need to search for documentation on dorny/paths-filter to verify extended glob pattern support.Let me search more specifically for picomatch extended glob support, since that's the underlying library:Verify that extglob negation patterns work correctly with paths-filter v3.0.2 before deployment.
Paths expressions are evaluated using picomatch library, with documentation for path expression format on the project GitHub page. While picomatch provides full support for standard and extended Bash glob features, including extglobs, braces, and POSIX brackets, the patterns
go/!(benchmarks|demo_api)/**andgo/!(benchmarks|demo_api)use extglob negation syntax rather than the leading-negation patterns (!pattern) shown in paths-filter's documented examples. No explicit test cases confirm this specific extglob negation syntax works as intended with paths-filter v3.0.2. Test these patterns in a non-production workflow run to ensure they correctly:
- Match paths like
go/apps/**,go/cmd/**,go/internal/**- Exclude
go/benchmarks/**andgo/demo_api/**
mcstepp
changed the title
1 participant
What does this PR do?
Fixes # (issue)
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated