introduce ParseKeyValueFile #5491
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5491 +/- ##
=======================================
Coverage 60.09% 60.10%
=======================================
Files 345 345
Lines 23445 23447 +2
=======================================
+ Hits 14090 14092 +2
Misses 8381 8381
Partials 974 974 |
thaJeztah
left a comment
There was a problem hiding this comment.
Left some comments; do you need this change urgently? If not, I want to give my ideas a quick try to see what it looks like
opts/envfile.go
Outdated
| // nothing more. | ||
| func ParseEnvFile(filename string) ([]string, error) { | ||
| return parseKeyValueFile(filename, os.LookupEnv) | ||
| return ParseEnvFileWithLookup(filename, os.LookupEnv) |
There was a problem hiding this comment.
I prefer for this one to continue using the non-exported (internal) function; it makes it clearer whether exported functions are used, and makes it clearer that the internal implementation can be changed.
opts/envfile.go
Outdated
| // ParseEnvFileWithLookup reads a file with environment variables enumerated by lines | ||
| func ParseEnvFileWithLookup(filename string, env func(key string) (string, bool)) ([]string, error) { | ||
| return parseKeyValueFile(filename, env) | ||
| } |
There was a problem hiding this comment.
Some thoughts here;
- Perhaps
lookupFnis a better name for this thanenv. - Can you update the GoDoc to describe the new functionality (i.e., a
lookupFncan be passed to provide values for the given key if it has no value set) - Can you add a minimal unit-test for this (using a custom lookup function)?
Also wondering;
- For compose's use; is it better to pass a filename or should we have a function that takes an
io.Reader? (Honestly, also looking at some of the existing unit-tests, where the requirement for having a file makes the tests more complicated than if it was a reader 😂) - Effectively, this is now identical to
parseKeyValueFile, which is a more agnostic name, and perhaps makes the overall functionality more "flexible" (if the format is to be used elsewhere).
Considering if we should move parseKeyValueFile to a be exported (accepting a reader) or even a separate package, so that it can be consumed without all of the other opts to be used (e.g. keyvaluefile.Parse())
There was a problem hiding this comment.
:+1 on using io.Reader (still need filename for error message). I tried to introduce the minimal required change here, but can make this change
There was a problem hiding this comment.
updated with ParseKeyValueFile to accept a Reader. Didn't moved this out of the opts package as error message refers to "environment variable" so this still make it somehow dedicated to env file parsing
045f6f5 to
90deee5
Compare
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
- What I did
introduced
ParseKeyValueFileso docker/compose can use the same parser as docker/cli when env_file is requested to be loaded inrawformat. This will be used so a user can get the same behavior usingdocker run --env-fileand composeenv_filewhile preserving the ability to get variable interpolated by default 😵💫- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)
