Implement the "source" side of profile parsing #594
Conversation
This commit adds support for the "source" side of profile file parsing: Given a filesystem/environment, what files should we actually be reading?
don't run rustfmt on windows
| } | ||
| } | ||
|
|
||
| /// Process Environment Abstraction |
There was a problem hiding this comment.
Recommend adding more details on what this means. I had to read the code to realize it's an abstraction over environment variables.
| @@ -9,6 +9,12 @@ license = "Apache-2.0" | |||
|
|
|||
| [dependencies] | |||
| lazy_static = "1" | |||
| tracing = "0.1.26" | |||
There was a problem hiding this comment.
Should be less specific on the version number?
There was a problem hiding this comment.
yeah good call, when we use tracing as a library 0.1 is probably what we want
| //! Abstractions for testing code that interacts with the process environment: | ||
| //! - Determining the current time | ||
| //! - Reading environment variables | ||
| //! - Reading from the file system |
There was a problem hiding this comment.
How does this look in the module summary line in the generated docs when it has bullet points like this?
There was a problem hiding this comment.
just renders as bullets on the module page
| } | ||
| } | ||
|
|
||
| pub fn read(&self, path: impl AsRef<Path>) -> std::io::Result<Vec<u8>> { |
There was a problem hiding this comment.
Should this be called read_to_end to allow for room for others in the future?
| } | ||
| } | ||
|
|
||
| impl From<HashMap<String, String>> for ProcessEnvironment { |
There was a problem hiding this comment.
Should this implement From<Iterator<(String, String)>>?
There was a problem hiding this comment.
maybe? although then you would need to convert back to an iterator if you had a hashmap, so I think HashMap is probably the right thing here
| /// Load a [Source](Source) from a given environment and filesystem. | ||
| pub fn load(proc_env: &environment::ProcessEnvironment, fs: &environment::Fs) -> Source { | ||
| let config = tracing::info_span!("load_config_file") | ||
| .in_scope(|| read(&fs, &proc_env, "~/.aws/config", "AWS_CONFIG_FILE")); |
There was a problem hiding this comment.
Wasn't obvious that read was replacing ~ on different OSes. Should we just pass in the .aws/config part and just always look in the user directory for it? Or have a lower-level read function that takes a full path, and a higher level one that resolves it?
Better yet, should we have an enum to represent the config and credentials files with a path() function?
There was a problem hiding this comment.
there's some wrinkles here that we also need to resolve the home directory from what the user might give us in an environment variable
There was a problem hiding this comment.
so having implicit home directory searching is probably going to create more complexity (since we still need generic home directory replacement)
There was a problem hiding this comment.
but I can split up the path resolution and the home directory resolution for clarity.
| @@ -4,6 +4,8 @@ | |||
| */ | |||
|
|
|||
| pub mod build_metadata; | |||
| pub mod environment; | |||
There was a problem hiding this comment.
Should we make these doc hidden and name this something like internal_env to avoid requiring it to have a stable interface?
|
LLVM talks about virtual filesystems. There is a virtual |
Co-authored-by: John DiSanti <jdisanti@amazon.com>
Co-authored-by: John DiSanti <jdisanti@amazon.com>
Issue #, if available: #210
Description of changes: This PR makes several steps towards support of profile file parsing and is probably best reviewed commit-by-commit for clarity.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.