diff --git a/crates/squawk/src/config.rs b/crates/squawk/src/config.rs index 26f56404..91cb77c0 100644 --- a/crates/squawk/src/config.rs +++ b/crates/squawk/src/config.rs @@ -2,7 +2,14 @@ use anyhow::{Context, Result}; use log::info; use serde::Deserialize; use squawk_linter::{Rule, Version}; -use std::{env, path::Path, path::PathBuf}; +use std::{ + env, + io::{self, IsTerminal}, + path::{Path, PathBuf}, + process, +}; + +use crate::{Command, DebugOption, Opts, Reporter, UploadToGithubArgs}; const FILE_NAME: &str = ".squawk.toml"; @@ -13,7 +20,7 @@ pub struct UploadToGitHubConfig { } #[derive(Debug, Default, Deserialize)] -pub struct Config { +pub struct ConfigFile { #[serde(default)] pub excluded_paths: Vec, #[serde(default)] @@ -26,7 +33,7 @@ pub struct Config { pub upload_to_github: UploadToGitHubConfig, } -impl Config { +impl ConfigFile { pub fn parse(custom_path: Option) -> Result> { let path = if let Some(path) = custom_path { Some(path) @@ -46,6 +53,107 @@ impl Config { } } +pub struct Config { + pub excluded_paths: Vec, + pub excluded_rules: Vec, + pub pg_version: Option, + pub assume_in_transaction: bool, + pub upload_to_github: UploadToGitHubConfig, + pub upload_to_github_args: Option, + pub no_error_on_unmatched_pattern: bool, + pub is_stdin: bool, + pub stdin_filepath: Option, + pub github_annotations: bool, + pub reporter: Reporter, + pub verbose: bool, + pub debug: Option, + pub path_patterns: Vec, +} + +impl Config { + pub fn from(opts: Opts) -> Config { + let conf = ConfigFile::parse(opts.config_path) + .unwrap_or_else(|e| { + eprintln!("Configuration error: {e}"); + process::exit(1); + }) + .unwrap_or_default(); + + // the --exclude flag completely overrides the configuration file. + let excluded_rules = if let Some(excluded_rules) = opts.excluded_rules { + excluded_rules + } else { + conf.excluded_rules.clone() + }; + + // the --exclude-path flag completely overrides the configuration file. + let excluded_paths = if let Some(excluded_paths) = opts.excluded_path { + excluded_paths + } else { + conf.excluded_paths.clone() + }; + let pg_version = if let Some(pg_version) = opts.pg_version { + Some(pg_version) + } else { + conf.pg_version + }; + + let assume_in_transaction = if opts.assume_in_transaction { + opts.assume_in_transaction + } else if opts.no_assume_in_transaction { + !opts.no_assume_in_transaction + } else { + conf.assume_in_transaction.unwrap_or_default() + }; + + let no_error_on_unmatched_pattern = if opts.no_error_on_unmatched_pattern { + opts.no_error_on_unmatched_pattern + } else { + // TODO: we should have config support for these + false + }; + + info!("pg version: {pg_version:?}"); + info!("excluded rules: {:?}", &excluded_rules); + info!("excluded paths: {:?}", &excluded_paths); + info!("assume in a transaction: {assume_in_transaction:?}"); + info!("no error on unmatched pattern: {no_error_on_unmatched_pattern:?}"); + + let is_stdin = !io::stdin().is_terminal(); + let github_annotations = std::env::var("GITHUB_ACTIONS").is_ok() + && std::env::var("SQUAWK_DISABLE_GITHUB_ANNOTATIONS").is_err(); + + // TODO: we should support all of these in the config file as well + let debug = opts.debug; + let verbose = opts.verbose; + let path_patterns = opts.path_patterns; + let reporter = opts.reporter.unwrap_or_default(); + let stdin_filepath = opts.stdin_filepath; + let upload_to_github = conf.upload_to_github; + let upload_to_github_args = match opts.cmd { + Some(Command::UploadToGithub(args)) => Some(*args), + _ => None, + }; + + Config { + excluded_paths, + excluded_rules, + pg_version, + assume_in_transaction, + upload_to_github, + upload_to_github_args, + no_error_on_unmatched_pattern, + is_stdin, + stdin_filepath, + github_annotations, + reporter, + verbose, + debug, + path_patterns, + } + } +} + fn recurse_directory(directory: &Path, file_name: &str) -> Result, std::io::Error> { for entry in directory.read_dir()? { let entry = entry?; @@ -85,7 +193,7 @@ assume_in_transaction = true "#; fs::write(&squawk_toml, file).expect("Unable to write file"); - assert_debug_snapshot!(Config::parse(Some(squawk_toml.path().to_path_buf()))); + assert_debug_snapshot!(ConfigFile::parse(Some(squawk_toml.path().to_path_buf()))); } #[test] fn load_pg_version() { @@ -95,7 +203,7 @@ pg_version = "19.1" "#; fs::write(&squawk_toml, file).expect("Unable to write file"); - assert_debug_snapshot!(Config::parse(Some(squawk_toml.path().to_path_buf()))); + assert_debug_snapshot!(ConfigFile::parse(Some(squawk_toml.path().to_path_buf()))); } #[test] fn load_excluded_rules() { @@ -105,7 +213,7 @@ excluded_rules = ["require-concurrent-index-creation"] "#; fs::write(&squawk_toml, file).expect("Unable to write file"); - assert_debug_snapshot!(Config::parse(Some(squawk_toml.path().to_path_buf()))); + assert_debug_snapshot!(ConfigFile::parse(Some(squawk_toml.path().to_path_buf()))); } #[test] fn load_excluded_paths() { @@ -115,7 +223,7 @@ excluded_paths = ["example.sql"] "#; fs::write(&squawk_toml, file).expect("Unable to write file"); - assert_debug_snapshot!(Config::parse(Some(squawk_toml.path().to_path_buf()))); + assert_debug_snapshot!(ConfigFile::parse(Some(squawk_toml.path().to_path_buf()))); } #[test] fn load_assume_in_transaction() { @@ -125,7 +233,7 @@ assume_in_transaction = false "; fs::write(&squawk_toml, file).expect("Unable to write file"); - assert_debug_snapshot!(Config::parse(Some(squawk_toml.path().to_path_buf()))); + assert_debug_snapshot!(ConfigFile::parse(Some(squawk_toml.path().to_path_buf()))); } #[test] fn load_fail_on_violations() { @@ -135,7 +243,7 @@ assume_in_transaction = false fail_on_violations = true "; fs::write(&squawk_toml, file).expect("Unable to write file"); - assert_debug_snapshot!(Config::parse(Some(squawk_toml.path().to_path_buf()))); + assert_debug_snapshot!(ConfigFile::parse(Some(squawk_toml.path().to_path_buf()))); } #[test] fn load_excluded_rules_with_alias() { @@ -145,6 +253,6 @@ excluded_rules = ["prefer-timestamp-tz", "prefer-timestamptz"] "#; fs::write(&squawk_toml, file).expect("Unable to write file"); - assert_debug_snapshot!(Config::parse(Some(squawk_toml.path().to_path_buf()))); + assert_debug_snapshot!(ConfigFile::parse(Some(squawk_toml.path().to_path_buf()))); } } diff --git a/crates/squawk/src/github.rs b/crates/squawk/src/github.rs index 513a7165..56730ebc 100644 --- a/crates/squawk/src/github.rs +++ b/crates/squawk/src/github.rs @@ -2,11 +2,10 @@ use crate::UploadToGithubArgs; use crate::config::Config; use crate::reporter::{CheckReport, fmt_github_annotations, fmt_tty_violation}; use crate::{file_finding::find_paths, reporter::check_files}; -use anyhow::{Result, anyhow, bail}; +use anyhow::{Context, Result, anyhow, bail}; use console::strip_ansi_codes; use log::info; use squawk_github::{GitHubApi, actions, app, comment_on_pr}; -use squawk_linter::{Rule, Version}; use std::io; const VERSION: &str = env!("CARGO_PKG_VERSION"); @@ -38,19 +37,21 @@ fn get_github_private_key( } fn create_gh_app( - github_api_url: Option, - github_install_id: Option, - github_app_id: Option, - github_token: Option, - github_private_key: Option, - github_private_key_base64: Option, + github_api_url: &Option, + github_install_id: &Option, + github_app_id: &Option, + github_token: &Option, + github_private_key: &Option, + github_private_key_base64: &Option, ) -> Result> { if let Some(github_install_id) = github_install_id { if let Some(github_app_id) = github_app_id { info!("using github app client"); - let gh_private_key = - get_github_private_key(github_private_key, github_private_key_base64)?; - let app = app::GitHub::new(&gh_private_key, github_app_id, github_install_id)?; + let gh_private_key = get_github_private_key( + github_private_key.clone(), + github_private_key_base64.clone(), + )?; + let app = app::GitHub::new(&gh_private_key, *github_app_id, *github_install_id)?; return Ok(Box::new(app)); } } @@ -58,8 +59,8 @@ fn create_gh_app( if let Some(github_token) = github_token { info!("using github actions client"); let client = match github_api_url { - Some(github_api_url) => actions::GitHub::new_with_url(&github_api_url, &github_token), - None => actions::GitHub::new(&github_token), + Some(github_api_url) => actions::GitHub::new_with_url(github_api_url, github_token), + None => actions::GitHub::new(github_token), }; return Ok(Box::new(client)); }; @@ -79,17 +80,10 @@ fn create_gh_app( const COMMENT_HEADER: &str = "# Squawk Report"; -pub fn check_and_comment_on_pr( - args: UploadToGithubArgs, - cfg: &Config, - is_stdin: bool, - stdin_path: Option, - exclude: &[Rule], - exclude_paths: &[String], - pg_version: Option, - assume_in_transaction: bool, - github_annotations: bool, -) -> Result<()> { +pub fn check_and_comment_on_pr(cfg: Config) -> Result<()> { + let args = cfg + .upload_to_github_args + .context("Should always have args for the github command")?; let UploadToGithubArgs { paths, fail_on_violations, @@ -112,24 +106,24 @@ pub fn check_and_comment_on_pr( }; let github_app = create_gh_app( - github_api_url, - github_install_id, - github_app_id, - github_token, - github_private_key, - github_private_key_base64, + &github_api_url, + &github_install_id, + &github_app_id, + &github_token, + &github_private_key, + &github_private_key_base64, )?; - let found_paths = find_paths(&paths, exclude_paths)?; + let found_paths = find_paths(&paths, &cfg.excluded_paths)?; info!("checking files"); let file_results = check_files( &found_paths, - is_stdin, - stdin_path, - exclude, - pg_version, - assume_in_transaction, + cfg.is_stdin, + &cfg.stdin_filepath, + &cfg.excluded_rules, + cfg.pg_version, + cfg.assume_in_transaction, )?; // We should only leave a comment when there are files checked. @@ -149,7 +143,7 @@ pub fn check_and_comment_on_pr( COMMENT_HEADER, )?; - if github_annotations { + if cfg.github_annotations { let stdout = io::stdout(); let mut handle = stdout.lock(); fmt_github_annotations(&mut handle, &file_results)?; diff --git a/crates/squawk/src/main.rs b/crates/squawk/src/main.rs index 6bef6215..8025ea59 100644 --- a/crates/squawk/src/main.rs +++ b/crates/squawk/src/main.rs @@ -4,17 +4,15 @@ mod file; mod file_finding; mod github; mod reporter; +use crate::config::Config; use crate::file_finding::find_paths; use anyhow::{Context, Result}; use clap::{CommandFactory, Parser, Subcommand, ValueEnum}; -use config::Config; use debug::debug; -use log::info; use reporter::check_and_dump_files; use simplelog::CombinedLogger; use squawk_linter::{Rule, Version}; use std::io; -use std::io::IsTerminal; use std::panic; use std::path::PathBuf; use std::process::{self, ExitCode}; @@ -70,8 +68,9 @@ pub enum DebugOption { Ast, } -#[derive(Debug, ValueEnum, Clone)] +#[derive(Debug, ValueEnum, Clone, Default)] pub enum Reporter { + #[default] Tty, Gcc, Json, @@ -82,7 +81,7 @@ pub enum Reporter { #[allow(clippy::struct_excessive_bools)] #[derive(Parser, Debug)] #[command(version)] -struct Opt { +struct Opts { /// Paths or patterns to search #[arg(value_name = "path")] path_patterns: Vec, @@ -165,7 +164,7 @@ Please open an issue at https://github.com/sbdchd/squawk/issues/new with the log writeln!(stderr, "{panic_info}\n{backtrace}\n{open_an_issue}").ok(); })); - let opts = Opt::parse(); + let opts = Opts::parse(); if opts.verbose { // ANSI codes don't render properly in the VSCode output pane @@ -183,112 +182,55 @@ Please open an issue at https://github.com/sbdchd/squawk/issues/new with the log .expect("problem creating logger"); } - let conf = Config::parse(opts.config_path) - .unwrap_or_else(|e| { - eprintln!("Configuration error: {e}"); - process::exit(1); - }) - .unwrap_or_default(); - - // the --exclude flag completely overrides the configuration file. - let excluded_rules = if let Some(excluded_rules) = opts.excluded_rules { - excluded_rules - } else { - conf.excluded_rules.clone() - }; - - // the --exclude-path flag completely overrides the configuration file. - let excluded_paths = if let Some(excluded_paths) = opts.excluded_path { - excluded_paths - } else { - conf.excluded_paths.clone() - }; - let pg_version = if let Some(pg_version) = opts.pg_version { - Some(pg_version) - } else { - conf.pg_version - }; - - let assume_in_transaction = if opts.assume_in_transaction { - opts.assume_in_transaction - } else if opts.no_assume_in_transaction { - !opts.no_assume_in_transaction - } else { - conf.assume_in_transaction.unwrap_or_default() - }; - - let no_error_on_unmatched_pattern = if opts.no_error_on_unmatched_pattern { - opts.no_error_on_unmatched_pattern - } else { - false - }; - - info!("pg version: {pg_version:?}"); - info!("excluded rules: {:?}", &excluded_rules); - info!("excluded paths: {:?}", &excluded_paths); - info!("assume in a transaction: {assume_in_transaction:?}"); - info!("no error on unmatched pattern: {no_error_on_unmatched_pattern:?}"); - - let is_stdin = !io::stdin().is_terminal(); - let github_annotations = std::env::var("GITHUB_ACTIONS").is_ok() - && std::env::var("SQUAWK_DISABLE_GITHUB_ANNOTATIONS").is_err(); match opts.cmd { Some(Command::Server) => { squawk_server::run().context("language server failed")?; } - Some(Command::UploadToGithub(args)) => { - github::check_and_comment_on_pr( - *args, - &conf, - is_stdin, - opts.stdin_filepath, - &excluded_rules, - &excluded_paths, - pg_version, - assume_in_transaction, - github_annotations, - ) - .context("Upload to GitHub failed")?; + Some(Command::UploadToGithub(_)) => { + let conf = Config::from(opts); + github::check_and_comment_on_pr(conf).context("Upload to GitHub failed")?; } None => { + let conf = Config::from(opts); + // TODO: do we need to do the same thing for the github command? let found_paths = - find_paths(&opts.path_patterns, &excluded_paths).unwrap_or_else(|e| { + find_paths(&conf.path_patterns, &conf.excluded_paths).unwrap_or_else(|e| { eprintln!("Failed to find files: {e}"); process::exit(1); }); - if found_paths.is_empty() && !opts.path_patterns.is_empty() { + if found_paths.is_empty() && !conf.path_patterns.is_empty() { eprintln!( "Failed to find files for provided patterns: {:?}", - opts.path_patterns + conf.path_patterns ); - if !no_error_on_unmatched_pattern { + if !conf.no_error_on_unmatched_pattern { process::exit(1); } } - if !found_paths.is_empty() || is_stdin { + if !found_paths.is_empty() || conf.is_stdin { let stdout = io::stdout(); let mut handle = stdout.lock(); - let read_stdin = found_paths.is_empty() && is_stdin; - if let Some(kind) = opts.debug { - debug(&mut handle, &found_paths, read_stdin, &kind, opts.verbose)?; + let read_stdin = found_paths.is_empty() && conf.is_stdin; + if let Some(kind) = conf.debug { + debug(&mut handle, &found_paths, read_stdin, &kind, conf.verbose)?; } else { - let reporter = opts.reporter.unwrap_or(Reporter::Tty); + let reporter = conf.reporter; let exit_code = check_and_dump_files( &mut handle, &found_paths, read_stdin, - opts.stdin_filepath, - &excluded_rules, - pg_version, - assume_in_transaction, + conf.stdin_filepath, + &conf.excluded_rules, + conf.pg_version, + conf.assume_in_transaction, &reporter, - github_annotations, + conf.github_annotations, )?; return Ok(exit_code); } - } else if !no_error_on_unmatched_pattern { - Opt::command().print_long_help()?; + } else if !conf.no_error_on_unmatched_pattern { + Opts::command().print_long_help()?; println!(); } } diff --git a/crates/squawk/src/reporter.rs b/crates/squawk/src/reporter.rs index de6a02bf..90a6329d 100644 --- a/crates/squawk/src/reporter.rs +++ b/crates/squawk/src/reporter.rs @@ -131,7 +131,7 @@ fn render_lint_error( pub fn check_files( path_patterns: &[PathBuf], read_stdin: bool, - stdin_path: Option, + stdin_path: &Option, excluded_rules: &[Rule], pg_version: Option, assume_in_transaction: bool, @@ -144,7 +144,7 @@ pub fn check_files( if sql.trim().is_empty() { info!("ignoring empty stdin"); } else { - let path = stdin_path.unwrap_or_else(|| "stdin".into()); + let path = stdin_path.clone().unwrap_or_else(|| "stdin".into()); let content = check_sql( &sql, &path, @@ -186,7 +186,7 @@ pub fn check_and_dump_files( let violations = check_files( path_patterns, read_stdin, - stdin_path, + &stdin_path, excluded_rules, pg_version, assume_in_transaction, diff --git a/crates/squawk/src/snapshots/squawk__config__test_config__load_assume_in_transaction.snap b/crates/squawk/src/snapshots/squawk__config__test_config__load_assume_in_transaction.snap index c5a9b625..5fb18c87 100644 --- a/crates/squawk/src/snapshots/squawk__config__test_config__load_assume_in_transaction.snap +++ b/crates/squawk/src/snapshots/squawk__config__test_config__load_assume_in_transaction.snap @@ -1,10 +1,10 @@ --- -source: crates/cli/src/config.rs -expression: "Config::parse(Some(squawk_toml.path().to_path_buf()))" +source: crates/squawk/src/config.rs +expression: "ConfigFile::parse(Some(squawk_toml.path().to_path_buf()))" --- Ok( Some( - Config { + ConfigFile { excluded_paths: [], excluded_rules: [], pg_version: None, diff --git a/crates/squawk/src/snapshots/squawk__config__test_config__load_cfg_full.snap b/crates/squawk/src/snapshots/squawk__config__test_config__load_cfg_full.snap index 26c13a29..35945b31 100644 --- a/crates/squawk/src/snapshots/squawk__config__test_config__load_cfg_full.snap +++ b/crates/squawk/src/snapshots/squawk__config__test_config__load_cfg_full.snap @@ -1,10 +1,10 @@ --- -source: crates/cli/src/config.rs -expression: "Config::parse(Some(squawk_toml.path().to_path_buf()))" +source: crates/squawk/src/config.rs +expression: "ConfigFile::parse(Some(squawk_toml.path().to_path_buf()))" --- Ok( Some( - Config { + ConfigFile { excluded_paths: [ "example.sql", ], diff --git a/crates/squawk/src/snapshots/squawk__config__test_config__load_excluded_paths.snap b/crates/squawk/src/snapshots/squawk__config__test_config__load_excluded_paths.snap index e1b381f8..0f70a3d3 100644 --- a/crates/squawk/src/snapshots/squawk__config__test_config__load_excluded_paths.snap +++ b/crates/squawk/src/snapshots/squawk__config__test_config__load_excluded_paths.snap @@ -1,10 +1,10 @@ --- -source: crates/cli/src/config.rs -expression: "Config::parse(Some(squawk_toml.path().to_path_buf()))" +source: crates/squawk/src/config.rs +expression: "ConfigFile::parse(Some(squawk_toml.path().to_path_buf()))" --- Ok( Some( - Config { + ConfigFile { excluded_paths: [ "example.sql", ], diff --git a/crates/squawk/src/snapshots/squawk__config__test_config__load_excluded_rules.snap b/crates/squawk/src/snapshots/squawk__config__test_config__load_excluded_rules.snap index af87ef10..7848a7ed 100644 --- a/crates/squawk/src/snapshots/squawk__config__test_config__load_excluded_rules.snap +++ b/crates/squawk/src/snapshots/squawk__config__test_config__load_excluded_rules.snap @@ -1,10 +1,10 @@ --- -source: crates/cli/src/config.rs -expression: "Config::parse(Some(squawk_toml.path().to_path_buf()))" +source: crates/squawk/src/config.rs +expression: "ConfigFile::parse(Some(squawk_toml.path().to_path_buf()))" --- Ok( Some( - Config { + ConfigFile { excluded_paths: [], excluded_rules: [ RequireConcurrentIndexCreation, diff --git a/crates/squawk/src/snapshots/squawk__config__test_config__load_excluded_rules_with_alias.snap b/crates/squawk/src/snapshots/squawk__config__test_config__load_excluded_rules_with_alias.snap index 4b601469..2155d8b7 100644 --- a/crates/squawk/src/snapshots/squawk__config__test_config__load_excluded_rules_with_alias.snap +++ b/crates/squawk/src/snapshots/squawk__config__test_config__load_excluded_rules_with_alias.snap @@ -1,10 +1,10 @@ --- source: crates/squawk/src/config.rs -expression: "Config::parse(Some(squawk_toml.path().to_path_buf()))" +expression: "ConfigFile::parse(Some(squawk_toml.path().to_path_buf()))" --- Ok( Some( - Config { + ConfigFile { excluded_paths: [], excluded_rules: [ PreferTimestampTz, diff --git a/crates/squawk/src/snapshots/squawk__config__test_config__load_fail_on_violations.snap b/crates/squawk/src/snapshots/squawk__config__test_config__load_fail_on_violations.snap index c220dd50..8337825e 100644 --- a/crates/squawk/src/snapshots/squawk__config__test_config__load_fail_on_violations.snap +++ b/crates/squawk/src/snapshots/squawk__config__test_config__load_fail_on_violations.snap @@ -1,10 +1,10 @@ --- -source: crates/cli/src/config.rs -expression: "Config::parse(Some(squawk_toml.path().to_path_buf()))" +source: crates/squawk/src/config.rs +expression: "ConfigFile::parse(Some(squawk_toml.path().to_path_buf()))" --- Ok( Some( - Config { + ConfigFile { excluded_paths: [], excluded_rules: [], pg_version: None, diff --git a/crates/squawk/src/snapshots/squawk__config__test_config__load_pg_version.snap b/crates/squawk/src/snapshots/squawk__config__test_config__load_pg_version.snap index 457fbcd3..aeeec497 100644 --- a/crates/squawk/src/snapshots/squawk__config__test_config__load_pg_version.snap +++ b/crates/squawk/src/snapshots/squawk__config__test_config__load_pg_version.snap @@ -1,10 +1,10 @@ --- -source: crates/cli/src/config.rs -expression: "Config::parse(Some(squawk_toml.path().to_path_buf()))" +source: crates/squawk/src/config.rs +expression: "ConfigFile::parse(Some(squawk_toml.path().to_path_buf()))" --- Ok( Some( - Config { + ConfigFile { excluded_paths: [], excluded_rules: [], pg_version: Some(