Skip to content

Commit c0eece4

Browse files
oltrepoltrep
authored
Better error handling (#40)
1 parent 9e2384d commit c0eece4

17 files changed

+777
-432
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
## Unreleased Changes
44

5+
- Improve error handling to reduces crashes and add more useful error messages ([#40](https://github.com/Roblox/foreman/pull/40))
56
- Add environment variable to override Foreman home directory ([#39](https://github.com/Roblox/foreman/pull/39))
67
- Support tools hosted on GitLab ([#31](https://github.com/Roblox/foreman/pull/31))
78
- Updated config format to support both GitHub and GitLab tools

‎src/aliaser.rs‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,12 +3,17 @@ use std::{
33
path::Path,
44
};
55

6-
use crate::fs;
6+
use crate::{
7+
error::{ForemanError, ForemanResult},
8+
fs,
9+
};
710

8-
pub fn add_self_alias(name: &str, bin_path: &Path) {
9-
let foreman_path = env::current_exe().unwrap();
11+
pub fn add_self_alias(name: &str, bin_path: &Path) -> ForemanResult<()> {
12+
let foreman_path = env::current_exe().map_err(|err| {
13+
ForemanError::io_error_with_context(err, "unable to obtain foreman executable location")
14+
})?;
1015
let mut alias_path = bin_path.to_owned();
1116
alias_path.push(format!("{}{}", name, EXE_SUFFIX));
1217

13-
fs::copy(foreman_path, alias_path).unwrap();
18+
fs::copy(foreman_path, alias_path).map(|_| ())
1419
}

‎src/auth_store.rs‎

Lines changed: 34 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,12 @@
1-
use std::{io, path::Path};
1+
use std::path::Path;
22

33
use serde::{Deserialize, Serialize};
4-
use toml_edit::{value, Document};
4+
use toml_edit::{value, Document, TomlError};
55

6-
use crate::fs;
6+
use crate::{
7+
error::{ForemanError, ForemanResult},
8+
fs,
9+
};
710

811
pub static DEFAULT_AUTH_CONFIG: &str = include_str!("../resources/default-auth.toml");
912

@@ -15,59 +18,47 @@ pub struct AuthStore {
1518
}
1619

1720
impl AuthStore {
18-
pub fn load(path: &Path) -> io::Result<Self> {
19-
log::debug!("Loading auth store...");
21+
pub fn load(path: &Path) -> ForemanResult<Self> {
22+
if let Some(contents) = fs::try_read(path)? {
23+
log::debug!("Loading auth store");
24+
let store: AuthStore = toml::from_slice(&contents)
25+
.map_err(|error| ForemanError::auth_parsing(path, error.to_string()))?;
2026

21-
match fs::read(path) {
22-
Ok(contents) => {
23-
let store: AuthStore = toml::from_slice(&contents).unwrap();
24-
25-
let mut found_credentials = false;
26-
if store.github.is_some() {
27-
log::debug!("Found GitHub credentials");
28-
found_credentials = true;
29-
}
30-
if store.gitlab.is_some() {
31-
log::debug!("Found GitLab credentials");
32-
found_credentials = true;
33-
}
34-
if !found_credentials {
35-
log::debug!("Found no credentials");
36-
}
37-
38-
Ok(store)
27+
let mut found_credentials = false;
28+
if store.github.is_some() {
29+
log::debug!("Found GitHub credentials");
30+
found_credentials = true;
31+
}
32+
if store.gitlab.is_some() {
33+
log::debug!("Found GitLab credentials");
34+
found_credentials = true;
3935
}
40-
Err(err) => {
41-
if err.kind() == io::ErrorKind::NotFound {
42-
Ok(AuthStore::default())
43-
} else {
44-
Err(err)
45-
}
36+
if !found_credentials {
37+
log::debug!("Found no credentials");
4638
}
39+
40+
Ok(store)
41+
} else {
42+
log::debug!("Auth store not found");
43+
Ok(AuthStore::default())
4744
}
4845
}
4946

50-
pub fn set_github_token(auth_file: &Path, token: &str) -> io::Result<()> {
47+
pub fn set_github_token(auth_file: &Path, token: &str) -> ForemanResult<()> {
5148
Self::set_token(auth_file, "github", token)
5249
}
5350

54-
pub fn set_gitlab_token(auth_file: &Path, token: &str) -> io::Result<()> {
51+
pub fn set_gitlab_token(auth_file: &Path, token: &str) -> ForemanResult<()> {
5552
Self::set_token(auth_file, "gitlab", token)
5653
}
5754

58-
fn set_token(auth_file: &Path, key: &str, token: &str) -> io::Result<()> {
59-
let contents = match fs::read_to_string(auth_file) {
60-
Ok(contents) => contents,
61-
Err(err) => {
62-
if err.kind() == io::ErrorKind::NotFound {
63-
DEFAULT_AUTH_CONFIG.to_owned()
64-
} else {
65-
return Err(err);
66-
}
67-
}
68-
};
55+
fn set_token(auth_file: &Path, key: &str, token: &str) -> ForemanResult<()> {
56+
let contents =
57+
fs::try_read_to_string(auth_file)?.unwrap_or_else(|| DEFAULT_AUTH_CONFIG.to_owned());
6958

70-
let mut store: Document = contents.parse().unwrap();
59+
let mut store: Document = contents
60+
.parse()
61+
.map_err(|err: TomlError| ForemanError::auth_parsing(auth_file, err.to_string()))?;
7162
store[key] = value(token);
7263

7364
let serialized = store.to_string();

‎src/config.rs‎

Lines changed: 28 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,13 @@
1-
use std::{collections::HashMap, env, fmt, io};
1+
use std::{collections::HashMap, env, fmt};
22

33
use semver::VersionReq;
44
use serde::{Deserialize, Serialize};
55

6-
use crate::{ci_string::CiString, fs, paths::ForemanPaths, tool_provider::Provider};
6+
use crate::{
7+
ci_string::CiString, error::ForemanError, fs, paths::ForemanPaths, tool_provider::Provider,
8+
};
79

8-
#[derive(Debug, PartialEq, Serialize, Deserialize)]
10+
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
911
#[serde(untagged)]
1012
pub enum ToolSpec {
1113
Github {
@@ -83,29 +85,28 @@ impl ConfigFile {
8385
}
8486
}
8587

86-
pub fn aggregate(paths: &ForemanPaths) -> io::Result<ConfigFile> {
88+
pub fn aggregate(paths: &ForemanPaths) -> Result<ConfigFile, ForemanError> {
8789
let mut config = ConfigFile::new();
8890

89-
let base_dir = env::current_dir()?;
91+
let base_dir = env::current_dir().map_err(|err| {
92+
ForemanError::io_error_with_context(
93+
err,
94+
"unable to obtain the current working directory",
95+
)
96+
})?;
9097
let mut current_dir = base_dir.as_path();
9198

9299
loop {
93100
let config_path = current_dir.join("foreman.toml");
94101

95-
match fs::read(&config_path) {
96-
Ok(contents) => {
97-
let config_source = toml::from_slice(&contents).unwrap();
98-
log::debug!(
99-
"aggregating content from config file at {}",
100-
config_path.display()
101-
);
102-
config.fill_from(config_source);
103-
}
104-
Err(err) => {
105-
if err.kind() != io::ErrorKind::NotFound {
106-
return Err(err);
107-
}
108-
}
102+
if let Some(contents) = fs::try_read(&config_path)? {
103+
let config_source = toml::from_slice(&contents)
104+
.map_err(|err| ForemanError::config_parsing(&config_path, err.to_string()))?;
105+
log::debug!(
106+
"aggregating content from config file at {}",
107+
config_path.display()
108+
);
109+
config.fill_from(config_source);
109110
}
110111

111112
if let Some(parent) = current_dir.parent() {
@@ -116,20 +117,14 @@ impl ConfigFile {
116117
}
117118

118119
let home_config_path = paths.user_config();
119-
match fs::read(&home_config_path) {
120-
Ok(contents) => {
121-
let config_source = toml::from_slice(&contents).unwrap();
122-
log::debug!(
123-
"aggregating content from config file at {}",
124-
home_config_path.display()
125-
);
126-
config.fill_from(config_source);
127-
}
128-
Err(err) => {
129-
if err.kind() != io::ErrorKind::NotFound {
130-
return Err(err);
131-
}
132-
}
120+
if let Some(contents) = fs::try_read(&home_config_path)? {
121+
let config_source = toml::from_slice(&contents)
122+
.map_err(|err| ForemanError::config_parsing(&home_config_path, err.to_string()))?;
123+
log::debug!(
124+
"aggregating content from config file at {}",
125+
home_config_path.display()
126+
);
127+
config.fill_from(config_source);
133128
}
134129

135130
Ok(config)

0 commit comments

Comments
 (0)