Search amber.yaml in parent directory in some cases #22
Conversation
This PR makes amber searches the parent directory for the amber.yaml file if amber.yaml isn't present in the current working directory. This check is only done when no explicit amber-yaml is specificed via --amber-yaml option or via the environment variable (unless the specified value matches the default amber.yaml value) Fixes fpco#21
src/config.rs
Outdated
| @@ -121,13 +121,19 @@ impl Config { | |||
| } | |||
|
|
|||
| pub fn load<P: AsRef<Path>>(path: P) -> Result<Self> { | |||
| let path = path.as_ref(); | |||
There was a problem hiding this comment.
You can bypass the need for unnecessary allocation below by using Cow:
pub fn load<P: AsRef<Path>>(path: P) -> Result<Self> {
let path = path.as_ref();
let load_path = if path == Path::new(crate::cli::DEFAULT_AMBER_YAML) {
Cow::Owned(find_amber_yaml(&path)?)
} else {
Cow::Borrowed(path)
};
let res: Result<Self> = (|| {
let mut file = fs_err::File::open(&*load_path)?;
let config = serde_yaml::from_reader(&mut file)?;
Config::from_raw(config)
})();
res.with_context(|| format!("Unable to read file {}", load_path.display()))
}
src/config.rs
Outdated
| let config = serde_yaml::from_reader(&mut file)?; | ||
| Config::from_raw(config) | ||
| })(); | ||
| res.with_context(|| format!("Unable to read file {}", path.display())) | ||
| res.with_context(|| format!("Unable to read file {}", load_path.display())) | ||
| } | ||
|
|
||
| pub fn save<P: AsRef<Path>>(&self, path: P) -> Result<()> { |
There was a problem hiding this comment.
It looks like the same logic from above would need to be applied to save. I'd rather move the logic earlier, perhaps when loading up the settings from the CLI.
src/config.rs
Outdated
| return Ok(amber_yaml); | ||
| } | ||
| } | ||
| bail!("No file named {} found", &name.as_ref().display()) |
There was a problem hiding this comment.
| bail!("No file named {} found", &name.as_ref().display()) | |
| Err(anyhow!("No file named {} found", &name.as_ref().display())) |
Not a big deal, but I'd prefer to avoid short-circuiting
src/config.rs
Outdated
| } | ||
| let path = std::env::current_dir()?; | ||
| for file in path.ancestors() { | ||
| let amber_yaml: PathBuf = file.join(&name); |
There was a problem hiding this comment.
Technically, from context, this code is fine, since name will always been amber.yaml. However, if this function was called with a Path that includes a directory, I think the behavior would be surprising. Instead of taking an argument here for the name, I think using the const value for the default file name would be better.
src/cli.rs
Outdated
| /// Utility to store encrypted secrets in version trackable plain text files. | ||
| #[derive(Clap, Debug)] | ||
| pub struct Opt { | ||
| /// Turn on verbose output | ||
| #[clap(short, long, global = true)] | ||
| pub verbose: bool, | ||
| /// amber.yaml file location | ||
| #[clap(long, default_value = "amber.yaml", global = true, env = "AMBER_YAML")] | ||
| #[clap(long, default_value = DEFAULT_AMBER_YAML, global = true, env = "AMBER_YAML")] |
There was a problem hiding this comment.
One downside to this approach: we cannot distinguish "explicit stated amber.yaml" from "didn't state anything." It's not a big deal, but I think an approach that captures that concept would be better. I took a stab at adding a method find_amber_yaml to Opt instead, which I think also addresses my concern below about the save functionality not being updated:
I'm not thrilled about the need for &mut self, but it's a reasonable thing to require in this case. What do you think?
There was a problem hiding this comment.
Thanks, the changes looks good to me. And yes, modelling it explicitly via Option is much better. I like your changes - I have left a single comment there.
There was a problem hiding this comment.
Cool, want to pull those changes into your branch to update this PR?
This PR makes amber searches the parent directory for the amber.yaml
file if amber.yaml isn't present in the current working
directory.
This check is only done when no explicit amber-yaml is specificed via
--amber-yaml option or via the environment variable (unless the
specified value matches the default amber.yaml value)
Fixes #21