Ability to load variable from file #614
Conversation
bradrydzewski
changed the title
|
The failed unit test is because I updated the documentation in the readme without realizing there were validations. I wasn't exactly sure how to fix, but if someone can point me in the right direction I can update the pr |
|
Awesome, that looks great |
|
@jszwedko friendly ping, looks like you are the most active maintainer. I was wondering your thoughts, and if I could get a review. Thanks! |
|
@bradrydzewski 👍 this is looking good, thanks for the contribution! My only thought is that we may want to more explicitly document in the README what the order of precedence of the sources is (I was surprised we didn't already, but it feels prudent to do so now given the addition of the new source). |
| @@ -32,6 +32,7 @@ applications in an expressive way. | |||
| + [Alternate Names](#alternate-names) | |||
| + [Ordering](#ordering) | |||
| + [Values from the Environment](#values-from-the-environment) | |||
| + [Values from files](#values-from-files) | |||
There was a problem hiding this comment.
Looks like the linter wants this to match the heading (capital "Files")
README.md
Outdated
|
|
||
| <!-- { | ||
| "args": ["--help"], | ||
| "output": "language for the greeting.*APP_LANG" |
There was a problem hiding this comment.
gfmrun expects the output specified here to match the program output when ran (this helps make the examples are not invalidated by code changes). In this particular case, I might suggest writing a temporary file in the example and then having a default action on app that prints out the value, updating this expected output to match.
You can test locally via:
go get github.com/urfave/gfmrun/...
./runtests gfmrun
There was a problem hiding this comment.
Also removing the --help from the args.
There was a problem hiding this comment.
You can find more details about gfmrun here: https://github.com/urfave/gfmrun#go
|
I am wondering if we should try to integrate this into the help output for flags with the |
|
Thanks for the review! I'll plan on updating the pr accordingly this coming week
I can certainly add this to the documentation. I also meant to solicit feedback on precedence. Should it be Flag > Env > File? Or Flag > File > Env? Given the primary use case for loading parameters from file (secrets) it made me wonder whether or not it should take precedence over environment, although I'll admit I'm having difficulty articulating why one would be more important than the other. Any thoughts? |
I added a section to document precedence for the file path. Let me know if you have any thoughts or feedback on the wording (writing good documentation is harder than writing code, IMO 😄 )
I also added the filepath to help. Below is an example output. Please let me know if you have any feedback regarding the formatting. |
|
friendly ping @jszwedko ... I'm sure you are busy so I want to do as much as I can on my end to help push this through. I think the PR addresses most (all?) of your comments, but it looks like travis is acting up. For the life of me I can't figure out what is going on. Any thoughts? Thanks so much!! |
|
Hi @bradrydzewski, Apologies for the delay in reviewing this! This looks good to me. Regarding the build failures, this appears to be due to some incompatibilities in the golang.org/x/tools tree for some Go versions. I'm working on fixing this over in #624. I'll drop a note here when that is merged. |
|
#624 has been merged, updating this branch |
|
And we're green, thanks again @bradrydzewski ! |
|
Actually, I have one more thought 😄 The other fields that are like this, e.g. |
|
yes, I'll update the pull request next week with that capability. Thanks! |
This demonstrates a potential solution for #613. If there is interest in possibly adding this feature we can flush out the implementation