Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions action.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,10 @@ inputs:
description: Please read the source code of flakehub-push before using this.
required: false
default: false
disable-github-api:
description: Disables communicating to the GitHub API.
required: false
default: false

# Used to construct the binary download URL
source-binary:
Expand Down
8 changes: 8 additions & 0 deletions src/cli/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,14 @@ pub(crate) struct FlakeHubPushCli {
)]
pub(crate) spdx_expression: OptionSpdxExpression,

#[clap(
long,
env = "FLAKEHUB_PUSH_DISABLE_GITHUB_API",
value_parser = EmptyBoolParser,
default_value_t = false
)]
pub(crate) disable_github_api: bool,

#[clap(
long,
env = "FLAKEHUB_PUSH_ERROR_ON_CONFLICT",
Expand Down
35 changes: 22 additions & 13 deletions src/git_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,43 +14,52 @@ pub struct GitContext {
impl GitContext {
pub fn from_cli_and_github(
cli: &FlakeHubPushCli,
github_graphql_data_result: &GithubGraphqlDataResult,
github_graphql_data_result: Option<GithubGraphqlDataResult>,
Copy link
Member

Choose a reason for hiding this comment

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

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

) -> Result<Self> {
// step: validate spdx, backfill from GitHub API
let spdx_expression = if cli.spdx_expression.0.is_none() {
if let Some(spdx_string) = &github_graphql_data_result.spdx_identifier {
if let Some(spdx_string) = github_graphql_data_result
.clone()
.and_then(|result| result.spdx_identifier.clone())
{
tracing::debug!("Recieved SPDX identifier `{}` from GitHub API", spdx_string);
let parsed = spdx::Expression::parse(spdx_string)
let parsed = spdx::Expression::parse(&spdx_string)
.wrap_err("Invalid SPDX license identifier reported from the GitHub API, either you are using a non-standard license or GitHub has returned a value that cannot be validated")?;
Some(parsed)
} else {
None
}
} else {
// Provide the user notice if the SPDX expression passed differs from the one detected on GitHub -- It's probably something they care about.
if github_graphql_data_result.spdx_identifier
!= cli.spdx_expression.0.as_ref().map(|v| v.to_string())
if github_graphql_data_result
.clone()
.map(|result| result.spdx_identifier)
!= Some(cli.spdx_expression.0.as_ref().map(|v| v.to_string()))
{
tracing::warn!(
"SPDX identifier `{}` was passed via argument, but GitHub's API suggests it may be `{}`",
cli.spdx_expression.0.as_ref().map(|v| v.to_string()).unwrap_or_else(|| "None".to_string()),
github_graphql_data_result.spdx_identifier.clone().unwrap_or_else(|| "None".to_string()),
github_graphql_data_result.clone().map(|result| result.spdx_identifier.clone().unwrap_or_else(|| "None".to_string())).unwrap_or_else(|| "None".to_string()),
)
}
cli.spdx_expression.0.clone()
};

let rev = cli
.rev
.0
.as_ref()
.unwrap_or(&github_graphql_data_result.revision);
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
});
Comment on lines +48 to +53
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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.

Suggested change
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).


let ctx = GitContext {
spdx_expression,
repo_topics: github_graphql_data_result.topics.clone(),
repo_topics: github_graphql_data_result
.clone()
.map(|result| result.topics.clone())
.unwrap_or_default(),
revision_info: RevisionInfo {
commit_count: Some(github_graphql_data_result.rev_count as usize),
commit_count: github_graphql_data_result.map(|result| result.rev_count as usize),
revision: rev.to_string(),
},
};
Expand Down
2 changes: 1 addition & 1 deletion src/github/graphql/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ impl GithubGraphqlDataQuery {
}
}

#[derive(Debug)]
#[derive(Debug, Clone)]
pub(crate) struct GithubGraphqlDataResult {
pub(crate) revision: String,
pub(crate) rev_count: i64,
Expand Down
36 changes: 24 additions & 12 deletions src/push_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,16 +78,22 @@ impl PushContext {
.clone()
.expect("failed to get github token when running in GitHub Actions");

let github_graphql_data_result = GithubGraphqlDataQuery::get(
&client,
&github_token,
&project_owner,
&project_name,
&local_rev_info.revision,
)
.await?;

let git_ctx = GitContext::from_cli_and_github(cli, &github_graphql_data_result)?;
let github_graphql_data_result = if cli.disable_github_api {
None
} else {
Some(
GithubGraphqlDataQuery::get(
&client,
&github_token,
&project_owner,
&project_name,
&local_rev_info.revision,
)
.await?,
)
};

let git_ctx = GitContext::from_cli_and_github(cli, github_graphql_data_result)?;

let token = crate::github::get_actions_id_bearer_token(&cli.host)
.await
Expand Down Expand Up @@ -131,8 +137,14 @@ impl PushContext {
)
.await?;

let git_ctx: GitContext =
GitContext::from_cli_and_github(cli, &github_graphql_data_result)?;
let git_ctx: GitContext = GitContext::from_cli_and_github(
cli,
if cli.disable_github_api {
None
} else {
Some(github_graphql_data_result.clone())
},
)?;

let token = flakehub_auth_fake::get_fake_bearer_token(
u,
Expand Down
4 changes: 4 additions & 0 deletions ts/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ type ExecutionEnvironment = {
FLAKEHUB_PUSH_ROLLING?: string;
FLAKEHUB_PUSH_MIRROR?: string;
FLAKEHUB_PUSH_ROLLING_MINOR?: string;
FLAKEHUB_PUSH_DISABLE_GITHUB_API?: string;
Copy link
Member

Choose a reason for hiding this comment

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

If they're using the GitHub Action, why would we want to disable the GitHub API?

Copy link
Member Author

Choose a reason for hiding this comment

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

useful if you add commits to a Flake that you don't want to push to GitHub but want in FlakeHub.

GITHUB_CONTEXT?: string;
};

Expand All @@ -50,6 +51,7 @@ class FlakeHubPushAction extends DetSysAction {
private mirror: boolean;
private name: string | null;
private rollingMinor: number | null;
private disableGitHubAPI: boolean;

constructor() {
super({
Expand Down Expand Up @@ -79,6 +81,7 @@ class FlakeHubPushAction extends DetSysAction {
this.mirror = inputs.getBool("mirror");
this.name = inputs.getStringOrNull("name");
this.rollingMinor = inputs.getNumberOrNull("rolling-minor");
this.disableGitHubAPI = inputs.getBool("disable-github-api");
}

async main(): Promise<void> {
Expand Down Expand Up @@ -133,6 +136,7 @@ class FlakeHubPushAction extends DetSysAction {
env.FLAKEHUB_PUSH_INCLUDE_OUTPUT_PATHS = this.includeOutputPaths.toString();
env.FLAKEHUB_PUSH_ROLLING = this.rolling.toString();
env.FLAKEHUB_PUSH_MIRROR = this.mirror.toString();
env.FLAKEHUB_PUSH_DISABLE_GITHUB_API = this.disableGitHubAPI.toString();

env.GITHUB_CONTEXT = JSON.stringify(actionsGithub.context);

Expand Down
Loading