Record events if authentication is skipped#198
Conversation
WalkthroughAdds an internal event constant Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant A as NixInstallerAction
participant C as ID Token Endpoints
participant T as Telemetry
U->>A: flakehubLogin()
A->>C: check for id-token endpoints
C-->>A: endpoints missing
alt PR is fork (base ≠ head)
A->>T: record EVENT_LOGIN_FAILURE (reason: "fork")
Note right of A: update message — FlakeHub disabled for fork
A-->>U: return (no login)
else Not configured
A->>T: record EVENT_LOGIN_FAILURE (reason: "not-configured")
Note right of A: update message — FlakeHub disabled, request id-token: write & contents: read
A-->>U: return (no login)
end
opt Login attempt
A->>C: attempt login
C-->>A: throws
A->>T: record EVENT_LOGIN_FAILURE (reason: "failed", details: error)
A-->>U: handle error and return
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (9)
✨ Finishing Touches
🧪 Generate unit tests
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 |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/index.ts (1)
21-24: Fix typo: success event constant equals the “start” name.
This causes mis-attribution of successful installs in telemetry.-const EVENT_INSTALL_NIX_SUCCESS = "install_nix_start"; +const EVENT_INSTALL_NIX_SUCCESS = "install_nix_success";
🧹 Nitpick comments (3)
src/index.ts (3)
730-736: Add minimal context to the “fork skip” event and harden fork detection.
Include PR number and repo names to aid triage; compare repo IDs (stable across renames) and ensure both sides exist before classifying as fork.- if (pr && base !== head) { - this.recordEvent(EVENT_AUTH_SKIP_FORK); + if (pr && pr.base?.repo?.id && pr.head?.repo?.id && pr.base.repo.id !== pr.head.repo.id) { + this.recordEvent(EVENT_AUTH_SKIP_FORK, { + prNumber: pr.number, + baseRepo: pr.base.repo.full_name, + headRepo: pr.head.repo.full_name, + }); actionsCore.info( `Not logging in to FlakeHub: GitHub Actions does not allow OIDC authentication from forked repositories ("${head}" is not the same repository as "${base}").`, ); return; }
739-747: Emit diagnostics with the “not-configured” skip event.
Capturing which endpoints are missing makes dashboards and support easier.- this.recordEvent(EVENT_AUTH_SKIP_NOT_CONFIGURED); + this.recordEvent(EVENT_AUTH_SKIP_NOT_CONFIGURED, { + missingUrl: !process.env["ACTIONS_ID_TOKEN_REQUEST_URL"], + missingToken: !process.env["ACTIONS_ID_TOKEN_REQUEST_TOKEN"], + });
721-724: Minor: make intent explicit by coercing to boolean.
Improves readability and avoids type inference as string/undefined.- const canLogin = - process.env["ACTIONS_ID_TOKEN_REQUEST_URL"] && - process.env["ACTIONS_ID_TOKEN_REQUEST_TOKEN"]; + const canLogin = Boolean( + process.env["ACTIONS_ID_TOKEN_REQUEST_URL"] && + process.env["ACTIONS_ID_TOKEN_REQUEST_TOKEN"] + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
dist/index.jsis excluded by!**/dist/**
📒 Files selected for processing (1)
src/index.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/index.ts (1)
dist/index.js (2)
head(100504-100504)base(100503-100503)
⏰ 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). (9)
- GitHub Check: Test: macos-14-large
- GitHub Check: Test: macos-14-xlarge with determinate
- GitHub Check: Test: macos-14-large with determinate
- GitHub Check: Test: nscloud-ubuntu-22.04-amd64-4x16
- GitHub Check: Test: macos-14-xlarge
- GitHub Check: Test: macos-13-large
- GitHub Check: Test: namespace-profile-default-arm64
- GitHub Check: Test: macos-13-large with determinate
- GitHub Check: Test: ubuntu-latest
🔇 Additional comments (1)
src/index.ts (1)
35-36: LGTM: new telemetry event identifiers look consistent and scoped.
Names align with existing colon-scoped events (e.g., debug-probe-urls:*). No issues.
Description
This PR adds
auth-skipevents of two types in order for us to be able to see possible repository issues. This should cut down on the misdiagnosis of authentication failures.Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Chores