Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to use git executable for fetching advisory database #420

Merged
merged 2 commits into from
May 16, 2022
Merged

Allow to use git executable for fetching advisory database #420

merged 2 commits into from
May 16, 2022

Conversation

danielhaap83
Copy link
Contributor

@danielhaap83 danielhaap83 commented May 6, 2022

This PR allows the use of the git executable directly instead of using git2 (fetching advisory database only!). This is similar to what cargo does and solves #419 for me.

The option can be selected from deny.toml's advisories section

[advisories]
# If this is true, then cargo deny will use the git executable to fetch advisory database.
# If this is false, then it uses a built-in git library.
# Setting this to true can be helpful if you have special authentication requirements that cargo-deny does not support.
# See Git Authentication for more information about setting up git authentication.
git-fetch-with-cli = true

Comment on lines 11 to 13
Allow,
/// If this is true, then cargo deny will use the git executable to fetch advisory database.
/// If this is false, then it uses a built-in git library.
Allow(bool),
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer if this was split into distinct variants, the reason the Fetch is an enum is because I avoid bool since it loses too much information, as shown by the need to comment what the bool means in this case.

fs::create_dir_all(parent)?;
}
} else {
return Err(anyhow!("invalid directory: {}", db_path.display()));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return Err(anyhow!("invalid directory: {}", db_path.display()));
anyhow::bail!("invalid directory: {}", db_path.display());


if let Some(parent) = db_path.parent() {
if !parent.is_dir() {
fs::create_dir_all(parent)?;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
fs::create_dir_all(parent)?;
fs::create_dir_all(parent).with_context(|| format!("failed to create advisory database directory {}", parent.display())?;

Copy link
Member

@Jake-Shadle Jake-Shadle left a comment

Choose a reason for hiding this comment

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

Great, thanks for the PR!

@mergify mergify bot merged commit 68595b3 into EmbarkStudios:main May 16, 2022
abernix added a commit to apollographql/router that referenced this pull request Jan 19, 2023
It uses its own escape hatch to force use of the CLI, much in the same way
that Cargo does.

This is critical for Windows.

Ref: EmbarkStudios/cargo-deny#420
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.

2 participants