From 92241d98adc184bf4b51ec044f6709c0e3090849 Mon Sep 17 00:00:00 2001 From: Cal Stepanian <61707847+cstepanian@users.noreply.github.com> Date: Mon, 17 Feb 2025 19:15:57 -0500 Subject: [PATCH] refactor: Improve clarity of ConfigError variants Resolves #931 Add new variants to replace all existing uses of `Unspecified`, other than `serde_json` parse errors: - `InternalError` - `FileNotFound` - `ParseError` - `EnvVarNotSet` - `MissingProgram` Signed-off-by: Cal Stepanian <61707847+cstepanian@users.noreply.github.com> --- .../proto/hipcheck/v1/hipcheck.proto | 10 ++++ plugins/activity/src/main.rs | 2 +- plugins/affiliation/src/main.rs | 11 ++--- plugins/binary/src/main.rs | 6 +-- plugins/churn/src/main.rs | 2 +- plugins/entropy/src/main.rs | 2 +- plugins/git/src/main.rs | 2 +- plugins/github/src/main.rs | 13 +++--- plugins/identity/src/main.rs | 2 +- plugins/linguist/src/main.rs | 4 +- plugins/review/src/main.rs | 2 +- plugins/typo/src/main.rs | 6 +-- sdk/rust/src/error.rs | 46 +++++++++++++++++++ 13 files changed, 80 insertions(+), 28 deletions(-) diff --git a/hipcheck-common/proto/hipcheck/v1/hipcheck.proto b/hipcheck-common/proto/hipcheck/v1/hipcheck.proto index 363a184c..a17a0ba5 100644 --- a/hipcheck-common/proto/hipcheck/v1/hipcheck.proto +++ b/hipcheck-common/proto/hipcheck/v1/hipcheck.proto @@ -108,6 +108,16 @@ enum ConfigurationStatus { CONFIGURATION_STATUS_UNRECOGNIZED_CONFIGURATION = 3; // The user provided a configuration item whose value is invalid. CONFIGURATION_STATUS_INVALID_CONFIGURATION_VALUE = 4; + // The plugin encountered an internal error, probably due to incorrect assumptions. + CONFIGURATION_STATUS_INTERNAL_ERROR = 5; + // A necessary plugin input file was not found. + CONFIGURATION_STATUS_FILE_NOT_FOUND = 6; + // The plugin's input data could not be parsed correctly. + CONFIGURATION_STATUS_PARSE_ERROR = 7; + // An environment variable needed by the plugin was not set. + CONFIGURATION_STATUS_ENV_VAR_NOT_SET = 8; + // The plugin could not find or run a needed program. + CONFIGURATION_STATUS_MISSING_PROGRAM = 9; } /*=========================================================================== diff --git a/plugins/activity/src/main.rs b/plugins/activity/src/main.rs index 65ab77b6..4aefce82 100644 --- a/plugins/activity/src/main.rs +++ b/plugins/activity/src/main.rs @@ -65,7 +65,7 @@ impl Plugin for ActivityPlugin { serde_json::from_value::(config).map_err(|e| ConfigError::Unspecified { message: e.to_string(), })?; - CONFIG.set(conf).map_err(|_e| ConfigError::Unspecified { + CONFIG.set(conf).map_err(|_e| ConfigError::InternalError { message: "config was already set".to_owned(), }) } diff --git a/plugins/affiliation/src/main.rs b/plugins/affiliation/src/main.rs index 5eaca2bf..04c50e9d 100644 --- a/plugins/affiliation/src/main.rs +++ b/plugins/affiliation/src/main.rs @@ -50,13 +50,10 @@ impl TryFrom for Config { if let Some(ofv) = value.orgs_file_path { // Get the Orgs file path and confirm it exists let orgs_file = PathBuf::from(&ofv); - file::exists(&orgs_file).map_err(|_e| ConfigError::Unspecified { - // Print error with Debug for full context - message: format!("could not find an orgs file at path {:?}", ofv), - })?; + file::exists(&orgs_file).map_err(|_e| ConfigError::FileNotFound { file_path: ofv })?; // Parse the Orgs file and construct an OrgSpec. let orgs_spec = - OrgSpec::load_from(&orgs_file).map_err(|e| ConfigError::Unspecified { + OrgSpec::load_from(&orgs_file).map_err(|e| ConfigError::ParseError { // Print error with Debug for full context message: format!("{:?}", e), })?; @@ -288,13 +285,13 @@ impl Plugin for AffiliationPlugin { // Store the policy conf to be accessed only in the `default_policy_expr()` impl self.policy_conf .set(conf.count_threshold) - .map_err(|_| ConfigError::Unspecified { + .map_err(|_| ConfigError::InternalError { message: "plugin was already configured".to_string(), })?; ORGSSPEC .set(conf.orgs_spec) - .map_err(|_e| ConfigError::Unspecified { + .map_err(|_e| ConfigError::InternalError { message: "orgs spec was already set".to_owned(), }) } diff --git a/plugins/binary/src/main.rs b/plugins/binary/src/main.rs index d5cb78a9..be847224 100644 --- a/plugins/binary/src/main.rs +++ b/plugins/binary/src/main.rs @@ -88,18 +88,18 @@ impl Plugin for BinaryPlugin { // Store the policy conf to be accessed only in the `default_policy_expr()` impl self.policy_conf .set(conf.opt_threshold) - .map_err(|_| ConfigError::Unspecified { + .map_err(|_| ConfigError::InternalError { message: "plugin was already configured".to_string(), })?; // Use the langs file to create a SourceFileDetector and init the salsa db let bfd = - BinaryFileDetector::load(conf.binary_file).map_err(|e| ConfigError::Unspecified { + BinaryFileDetector::load(conf.binary_file).map_err(|e| ConfigError::ParseError { message: e.to_string_pretty_multiline(), })?; // Make the salsa db globally accessible - DETECTOR.set(bfd).map_err(|_e| ConfigError::Unspecified { + DETECTOR.set(bfd).map_err(|_e| ConfigError::InternalError { message: "config was already set".to_owned(), }) } diff --git a/plugins/churn/src/main.rs b/plugins/churn/src/main.rs index b6904b4d..fe05ec96 100644 --- a/plugins/churn/src/main.rs +++ b/plugins/churn/src/main.rs @@ -261,7 +261,7 @@ impl Plugin for ChurnPlugin { // Store the PolicyExprConf to be accessed only in the `default_policy_expr()` impl self.policy_conf .set(conf.opt_policy) - .map_err(|_| ConfigError::Unspecified { + .map_err(|_| ConfigError::InternalError { message: "plugin was already configured".to_string(), }) } diff --git a/plugins/entropy/src/main.rs b/plugins/entropy/src/main.rs index 99630336..dea08351 100644 --- a/plugins/entropy/src/main.rs +++ b/plugins/entropy/src/main.rs @@ -169,7 +169,7 @@ impl Plugin for EntropyPlugin { // Store the PolicyExprConf to be accessed only in the `default_policy_expr()` impl self.policy_conf .set(conf.opt_policy) - .map_err(|_| ConfigError::Unspecified { + .map_err(|_| ConfigError::InternalError { message: "plugin was already configured".to_string(), }) } diff --git a/plugins/git/src/main.rs b/plugins/git/src/main.rs index 217428b4..ae563410 100644 --- a/plugins/git/src/main.rs +++ b/plugins/git/src/main.rs @@ -335,7 +335,7 @@ impl Plugin for GitPlugin { CACHE .set(Mutex::new(LruCache::new(NonZero::new(cache_size).unwrap()))) - .map_err(|_e| ConfigError::Unspecified { + .map_err(|_e| ConfigError::InternalError { message: "config was already set".to_owned(), }) } diff --git a/plugins/github/src/main.rs b/plugins/github/src/main.rs index b53ad22c..0fdbbe3a 100644 --- a/plugins/github/src/main.rs +++ b/plugins/github/src/main.rs @@ -32,12 +32,11 @@ impl TryFrom for Config { type Error = ConfigError; fn try_from(value: RawConfig) -> StdResult { if let Some(atv) = value.api_token_var { - let api_token = - std::env::var(&atv).map_err(|_e| ConfigError::InvalidConfigValue { - field_name: "api-token-var".to_owned(), - value: atv.clone(), - reason: format!("Could not find an environment variable with the name \"{}\". This environment variable must contain a GitHub API token.", atv), - })?; + let api_token = std::env::var(&atv).map_err(|_e| ConfigError::EnvVarNotSet { + env_var_name: atv.clone(), + field_name: "api-token-var".to_owned(), + purpose: "This environment variable must contain a GitHub API token.".to_owned(), + })?; Ok(Config { api_token }) } else { Err(ConfigError::MissingRequiredConfig { @@ -129,7 +128,7 @@ impl Plugin for GithubAPIPlugin { message: e.to_string(), })? .try_into()?; - CONFIG.set(conf).map_err(|_e| ConfigError::Unspecified { + CONFIG.set(conf).map_err(|_e| ConfigError::InternalError { message: "config was already set".to_owned(), }) } diff --git a/plugins/identity/src/main.rs b/plugins/identity/src/main.rs index 8d62b139..4e38c2f6 100644 --- a/plugins/identity/src/main.rs +++ b/plugins/identity/src/main.rs @@ -115,7 +115,7 @@ impl Plugin for IdentityPlugin { })?; self.policy_conf .set(conf.percent_threshold) - .map_err(|_| ConfigError::Unspecified { + .map_err(|_| ConfigError::InternalError { message: "plugin was already configured".to_string(), })?; Ok(()) diff --git a/plugins/linguist/src/main.rs b/plugins/linguist/src/main.rs index 9f865d38..c21a22ad 100644 --- a/plugins/linguist/src/main.rs +++ b/plugins/linguist/src/main.rs @@ -42,7 +42,7 @@ impl Plugin for LinguistPlugin { message: e.to_string(), })?; let sfd = match conf.langs_file { - Some(p) => SourceFileDetector::load(p).map_err(|e| ConfigError::Unspecified { + Some(p) => SourceFileDetector::load(p).map_err(|e| ConfigError::ParseError { message: e.to_string_pretty_multiline(), })?, None => { @@ -53,7 +53,7 @@ impl Plugin for LinguistPlugin { }); } }; - DETECTOR.set(sfd).map_err(|_e| ConfigError::Unspecified { + DETECTOR.set(sfd).map_err(|_e| ConfigError::InternalError { message: "config was already set".to_owned(), }) } diff --git a/plugins/review/src/main.rs b/plugins/review/src/main.rs index 1109ad33..6b087458 100644 --- a/plugins/review/src/main.rs +++ b/plugins/review/src/main.rs @@ -79,7 +79,7 @@ impl Plugin for ReviewPlugin { serde_json::from_value::(config).map_err(|e| ConfigError::Unspecified { message: e.to_string(), })?; - CONFIG.set(conf).map_err(|_e| ConfigError::Unspecified { + CONFIG.set(conf).map_err(|_e| ConfigError::InternalError { message: "config was already set".to_owned(), }) } diff --git a/plugins/typo/src/main.rs b/plugins/typo/src/main.rs index 1bfa3980..74fd0bef 100644 --- a/plugins/typo/src/main.rs +++ b/plugins/typo/src/main.rs @@ -48,7 +48,7 @@ impl TryFrom for Config { let typo_file = TypoFile::load_from(&typo_path).map_err(|e| { // Print error with Debug for full context log::error!("{:?}", e); - ConfigError::Unspecified { + ConfigError::ParseError { // Print error with Debug for full context message: format!("{:?}", e), } @@ -125,13 +125,13 @@ impl Plugin for TypoPlugin { // Store the policy conf to be accessed only in the `default_policy_expr()` impl self.policy_conf .set(conf.count_threshold) - .map_err(|_| ConfigError::Unspecified { + .map_err(|_| ConfigError::InternalError { message: "plugin was already configured".to_string(), })?; TYPOFILE .set(conf.typo_file) - .map_err(|_e| ConfigError::Unspecified { + .map_err(|_e| ConfigError::InternalError { message: "config was already set".to_owned(), }) } diff --git a/sdk/rust/src/error.rs b/sdk/rust/src/error.rs index 95858585..ef798a38 100644 --- a/sdk/rust/src/error.rs +++ b/sdk/rust/src/error.rs @@ -138,6 +138,28 @@ pub enum ConfigError { /// An unspecified error Unspecified { message: String }, + + /// The plugin encountered an error, probably due to incorrect assumptions. + InternalError { message: String }, + + /// A necessary plugin input file was not found. + FileNotFound { file_path: String }, + + /// The plugin's input data could not be parsed correctly. + ParseError { message: String }, + + /// An environment variable needed by the plugin was not set. + EnvVarNotSet { + /// Name of the environment variable + env_var_name: String, + /// Config field that set the variable name + field_name: String, + /// Message describing what the environment variable should contain + purpose: String, + }, + + /// The plugin could not run a needed program. + MissingProgram { program_name: String }, } impl From for SetConfigurationResponse { @@ -192,6 +214,30 @@ impl From for SetConfigurationResponse { status: ConfigurationStatus::Unspecified as i32, message, }, + ConfigError::InternalError { message } => SetConfigurationResponse { + status: ConfigurationStatus::InternalError as i32, + message: format!("The plugin encountered an error, probably due to incorrect assumptions: {message}"), + }, + ConfigError::FileNotFound { file_path } => SetConfigurationResponse { + status: ConfigurationStatus::FileNotFound as i32, + message: format!("File not found at path {file_path}"), + }, + ConfigError::ParseError { message } => SetConfigurationResponse { + status: ConfigurationStatus::ParseError as i32, + message: format!("The plugin's input data could not be parsed correctly: {message}"), + }, + ConfigError::EnvVarNotSet { + env_var_name, + field_name, + purpose, + } => SetConfigurationResponse { + status: ConfigurationStatus::EnvVarNotSet as i32, + message: format!("Could not find an environment variable with the name \"{env_var_name}\" (set by config field '{field_name}'). Purpose: {purpose}"), + }, + ConfigError::MissingProgram { program_name } => SetConfigurationResponse { + status: ConfigurationStatus::MissingProgram as i32, + message: format!("The plugin could not find or run a needed program: {program_name}"), + }, } } }