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.

Sign up for GitHub

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

Add option to fail on error when running with upload-to-github #327

Merged
merged 5 commits into from
Dec 9, 2023
Merged

Add option to fail on error when running with upload-to-github #327

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e7a76b0
add option to fail on error when running with upload-to-github
wmartins Dec 7, 2023
fcc2dca
Avoid calling comment_on_pr twice if both github_install_id and githu…
wmartins Dec 9, 2023
e5de367
Remove is_empty condition to exit when there are rule violations
wmartins Dec 9, 2023
daf8c28
error only on violations
chdsbd Dec 9, 2023
c97702f
bump version. add changelog endtry
chdsbd Dec 9, 2023
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
18 changes: 15 additions & 3 deletions cli/src/subcommand.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ pub enum SquawkError {
GithubPrivateKeyBase64DecodeError(base64::DecodeError),
GithubPrivateKeyDecodeError(std::string::FromUtf8Error),
GithubPrivateKeyMissing,
RulesViolatedError,
}

impl std::fmt::Display for SquawkError {
Expand All @@ -31,6 +32,7 @@ impl std::fmt::Display for SquawkError {
write!(f, "Could not decode GitHub private key to string: {err}")
}
Self::GithubPrivateKeyMissing => write!(f, "Missing GitHub private key"),
Self::RulesViolatedError => write!(f, "Rules were violated"),
}
}
}
Expand Down Expand Up @@ -59,6 +61,9 @@ pub enum Command {
/// --exclude=require-concurrent-index-creation,ban-drop-database
#[structopt(short, long, use_delimiter = true)]
exclude: Option<Vec<RuleViolationKind>>,
/// Exits with an error code when specified
#[structopt(long)]
exit_on_error: bool,
Copy link
Owner

Choose a reason for hiding this comment

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

thoughts on calling it something like:

Suggested change
exit_on_error: bool,
exit_on_violations: bool,

or maybe

Suggested change
exit_on_error: bool,
exit_with_code_on_violations: bool,

since we do exit with an error already, just not when there's violations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which one do you prefer? Another option on the table would be instead of using exit using something like fail: fail_on_violations

Copy link
Owner

Choose a reason for hiding this comment

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

Oh I like fail_on_violations!!

#[structopt(long, env = "SQUAWK_GITHUB_PRIVATE_KEY")]
github_private_key: Option<String>,
#[structopt(long, env = "SQUAWK_GITHUB_PRIVATE_KEY_BASE64")]
Expand Down Expand Up @@ -115,6 +120,7 @@ pub fn check_and_comment_on_pr(
let Command::UploadToGithub {
paths,
exclude,
exit_on_error,
github_private_key,
github_token,
github_app_id,
Expand All @@ -133,7 +139,8 @@ pub fn check_and_comment_on_pr(
pg_version,
assume_in_transaction,
)?;
if file_results.is_empty() {
let is_empty = file_results.is_empty();
if is_empty {
info!("no files checked, exiting");
return Ok(());
}
Expand All @@ -147,13 +154,13 @@ pub fn check_and_comment_on_pr(
get_github_private_key(github_private_key, github_private_key_base64)?;
let gh = app::GitHub::new(&gh_private_key, github_app_id, github_install_id)?;

return Ok(comment_on_pr(
comment_on_pr(
&gh,
&github_repo_owner,
&github_repo_name,
github_pr_number,
&comment_body,
)?);
)?;
}
}
if let Some(github_token) = github_token {
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
if let Some(github_token) = github_token {
else if let Some(github_token) = github_token {

so we don't have a case where we could have commont_on_pr run twice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I missed that, thanks for mentioning. Done in fcc2dca.

Expand All @@ -167,5 +174,10 @@ pub fn check_and_comment_on_pr(
&comment_body,
)?;
}

if !is_empty && exit_on_error {
Copy link
Owner

Choose a reason for hiding this comment

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

isn't !is_empty always true because we'll return on line 143 if is_empty is true?

Suggested change
if !is_empty && exit_on_error {
if exit_on_error {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're absolutely right! Done in e5de367.

return Err(SquawkError::RulesViolatedError);
}

Ok(())
}
Loading