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` - `ConfigFileParseError` - `EnvVarNotSet` - `MissingProgram` Signed-off-by: Cal Stepanian <[email protected]>
mitre · Feb 19, 2025 · 2836d05 · 2836d05
1 parent 64eeef0
commit 2836d05
Show file tree

Hide file tree

Showing 13 changed files with 87 additions and 32 deletions.
diff --git 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;
+    // A configuration file could not be parsed correctly.
+    CONFIGURATION_STATUS_CONFIG_FILE_PARSE_ERROR = 7;
+    // An environment variable needed by the plugin was not set.
+    CONFIGURATION_STATUS_ENV_VAR_NOT_SET = 8;
+    // The plugin could not run a needed program.
+    CONFIGURATION_STATUS_MISSING_PROGRAM = 9;
 }
 
 /*===========================================================================

diff --git 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>(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
@@ -50,13 +50,10 @@ impl TryFrom<RawConfig> 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::ConfigFileParseError {
 					// 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
@@ -88,18 +88,19 @@ 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 {
+		let bfd = BinaryFileDetector::load(conf.binary_file).map_err(|e| {
+			ConfigError::ConfigFileParseError {
 				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
@@ -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
@@ -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
@@ -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
@@ -32,12 +32,11 @@ impl TryFrom<RawConfig> for Config {
 	type Error = ConfigError;
 	fn try_from(value: RawConfig) -> StdResult<Config, ConfigError> {
 		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
@@ -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
@@ -42,9 +42,11 @@ impl Plugin for LinguistPlugin {
 				message: e.to_string(),
 			})?;
 		let sfd = match conf.langs_file {
-			Some(p) => SourceFileDetector::load(p).map_err(|e| ConfigError::Unspecified {
-				message: e.to_string_pretty_multiline(),
-			})?,
+			Some(p) => {
+				SourceFileDetector::load(p).map_err(|e| ConfigError::ConfigFileParseError {
+					message: e.to_string_pretty_multiline(),
+				})?
+			}
 			None => {
 				return Err(ConfigError::MissingRequiredConfig {
 					field_name: "langs-file".to_owned(),
@@ -53,7 +55,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
@@ -79,7 +79,7 @@ impl Plugin for ReviewPlugin {
 			serde_json::from_value::<Config>(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
@@ -48,7 +48,7 @@ impl TryFrom<RawConfig> 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::ConfigFileParseError {
 				// 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
@@ -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 },
+
+	/// A configuration file could not be parsed correctly.
+	ConfigFileParseError { 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<ConfigError> for SetConfigurationResponse {
@@ -192,6 +214,30 @@ impl From<ConfigError> 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::ConfigFileParseError { message } => SetConfigurationResponse {
+				status: ConfigurationStatus::ConfigFileParseError as i32,
+				message: format!("A configuration file 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 run a needed program: {program_name}"),
+			},
 		}
 	}
 }