Conversation
WalkthroughThis PR introduces a new Changes
Sequence DiagramsequenceDiagram
participant User as User/CI
participant Action as GitHub Action
participant CLI as CLI Handler
participant PushCtx as PushContext
participant GitCtx as GitContext
participant GitHub as GitHub API
User->>Action: disable-github-api flag
Action->>CLI: FLAKEHUB_PUSH_DISABLE_GITHUB_API env var
alt GitHub API Enabled
CLI->>PushCtx: disable_github_api=false
PushCtx->>GitHub: Fetch GraphQL data
GitHub-->>PushCtx: GithubGraphqlDataResult
PushCtx->>GitCtx: Some(github_data)
GitCtx->>GitCtx: Use GitHub data fields
else GitHub API Disabled
CLI->>PushCtx: disable_github_api=true
PushCtx->>GitCtx: None
GitCtx->>GitCtx: Use CLI/local data only
end
GitCtx-->>PushCtx: GitContext
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/git_context.rs (1)
21-23: Consider reducing repeated clones of github_graphql_data_result.The
github_graphql_data_resultis cloned multiple times throughout this function (lines 22, 35, 42, 50, 58). While this works correctly, consider usingas_ref()to avoid unnecessary clones where possible.For example, lines 21-23 could be rewritten as:
if let Some(spdx_string) = github_graphql_data_result .as_ref() .and_then(|result| result.spdx_identifier.as_ref()) {Similar optimizations could be applied to other usages. The last usage at line 62 can already move the value since it's the final use.
Also applies to: 34-37, 42-42, 50-50, 57-60
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
action.yaml(1 hunks)src/cli/mod.rs(1 hunks)src/git_context.rs(1 hunks)src/github/graphql/mod.rs(1 hunks)src/push_context.rs(2 hunks)ts/index.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
ts/index.ts (1)
dist/index.js (2)
inputs(3369-3371)env(99515-99515)
src/push_context.rs (2)
src/github/graphql/mod.rs (1)
get(26-143)src/git_context.rs (1)
from_cli_and_github(15-67)
⏰ 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: DeterminateCI / build (x86_64-linux, UbuntuLatest32Cores128G)
- GitHub Check: DeterminateCI / build (aarch64-linux, UbuntuLatest32Cores128GArm)
- GitHub Check: Check the dist/ folder is up to date
🔇 Additional comments (7)
action.yaml (1)
91-94: LGTM!The new
disable-github-apiinput is properly defined with an appropriate default value that maintains backward compatibility.src/github/graphql/mod.rs (1)
146-146: LGTM!Adding the
Clonederive is necessary to support the new optional data flow whereGithubGraphqlDataResultis wrapped inOptionand cloned when passed to various functions.src/cli/mod.rs (1)
91-97: LGTM!The new CLI flag follows established patterns and is properly configured with environment variable support and an appropriate default value.
ts/index.ts (1)
30-30: LGTM!The TypeScript changes properly integrate the new
disable-github-apiflag following the established patterns for other configuration options.Also applies to: 54-54, 84-84, 139-139
src/push_context.rs (2)
81-94: GitHub execution path correctly implements conditional API fetching.The implementation properly skips the GitHub API call when
disable_github_apiis true, returningNoneinstead.
123-157: Verify intentional design and consider documentation for LocalGitHub GitHub API requirement.The GitHub API is always called in
LocalGitHubmode regardless ofdisable_github_api, which is intentional: the flag only controls whether the fetched data is passed toGitContext, but fake token generation (line 153) requires the GitHub data as a non-optional parameter. This differs from theGitHubenvironment wheredisable_github_apican skip the API call entirely.Since
LocalGitHubis a development-only mode that mints fake JWTs for local testing, this design is correct. However, confirm whether this distinction should be:
- Documented in code comments or the action description
- Validated with a runtime error if
disable_github_apiis true inLocalGitHubmode- Or refactored to support fake token generation without GitHub data
src/git_context.rs (1)
15-18: Signature change properly supports optional GitHub data.The updated signature correctly reflects that GitHub data may not be available when the API is disabled.
| let rev = cli.rev.0.clone().unwrap_or_else(|| { | ||
| github_graphql_data_result | ||
| .clone() | ||
| .expect("GitHub API is disabled and rev is not specified") | ||
| .revision | ||
| }); |
There was a problem hiding this comment.
Replace panic with proper error handling.
Using .expect() causes a panic when both cli.rev and GitHub data are missing. This should return a proper error instead.
Apply this diff:
- let rev = cli.rev.0.clone().unwrap_or_else(|| {
- github_graphql_data_result
- .clone()
- .expect("GitHub API is disabled and rev is not specified")
- .revision
- });
+ let rev = match cli.rev.0.clone() {
+ Some(r) => r,
+ None => github_graphql_data_result
+ .clone()
+ .ok_or_else(|| eyre!("Could not determine revision: GitHub API is disabled and --rev was not specified"))?
+ .revision,
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let rev = cli.rev.0.clone().unwrap_or_else(|| { | |
| github_graphql_data_result | |
| .clone() | |
| .expect("GitHub API is disabled and rev is not specified") | |
| .revision | |
| }); | |
| let rev = match cli.rev.0.clone() { | |
| Some(r) => r, | |
| None => github_graphql_data_result | |
| .clone() | |
| .ok_or_else(|| eyre!("Could not determine revision: GitHub API is disabled and --rev was not specified"))? | |
| .revision, | |
| }; |
🤖 Prompt for AI Agents
In src/git_context.rs around lines 48 to 53, the use of .expect() inside
unwrap_or_else can panic if both cli.rev and github_graphql_data_result are
missing; change this to return a proper error instead. Replace the current
unwrap_or_else block so that you compute the revision as an Option
(cli.rev.0.clone().or_else(|| github_graphql_data_result.clone().map(|g|
g.revision))) and then convert that Option into a Result with ok_or_else(...)
returning a descriptive error (e.g., "revision not specified and GitHub API data
unavailable"), propagating it with ?; use the crate's existing error type (or
anyhow::Error) to match the function signature. Ensure you remove the .expect()
call and adjust the surrounding code to handle the Result return (propagate the
error up).
| pub fn from_cli_and_github( | ||
| cli: &FlakeHubPushCli, | ||
| github_graphql_data_result: &GithubGraphqlDataResult, | ||
| github_graphql_data_result: Option<GithubGraphqlDataResult>, |
There was a problem hiding this comment.
You should probably just make another function that doesn't accept GithubGraphqlDataResult as an arg instead of making this optional. The function is from_cli_and_github, not from_cli_and_maybe_github_if_reasons ;P
| FLAKEHUB_PUSH_ROLLING?: string; | ||
| FLAKEHUB_PUSH_MIRROR?: string; | ||
| FLAKEHUB_PUSH_ROLLING_MINOR?: string; | ||
| FLAKEHUB_PUSH_DISABLE_GITHUB_API?: string; |
There was a problem hiding this comment.
If they're using the GitHub Action, why would we want to disable the GitHub API?
There was a problem hiding this comment.
useful if you add commits to a Flake that you don't want to push to GitHub but want in FlakeHub.
2 participants
Adds a method to bypass the GitHub API, useful if you add commits to a Flake that you don't want to push to GitHub but want in FlakeHub.
Summary by CodeRabbit
disable-github-apioption to disable GitHub API communication during execution. Configure via CLI flag, action input, orFLAKEHUB_PUSH_DISABLE_GITHUB_APIenvironment variable.