feat: File config #2005
Merged
feat: File config #2005
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
geomodular
reviewed
Jun 6, 2023
geomodular
approved these changes
Jun 7, 2023
Contributor
geomodular
left a comment
There was a problem hiding this comment.
Good job Martin 👍 . I'm not really a fan of an intermediate Conf structure, but I get you cannot just use it to run the server.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds an option to configure waved via configuration (
.env) file in addition to existing cmd args and env variables.Disclaimer
I realized Wave is pretty special-case in a sense that it uses kebabcase (dashes) for cmd args and snakecase for env vars. So I added a kebabToSnake flag that deals with this inconsistency. However, I later realized such special-casing would need to be introduced on .env plugin I also contributed, which may be a bit confusing for people since it would be the only plugin supporting the kebabToSnake flag. For this reason, I decided to include the .env plugin within Wave code.
TODO
Wave app can only be configured via env variables since we recommend running bare uvicorn instead of using our
wave runwrapper.Uvicorn supports
--env-fileflag to allow configuration for the underlying (Wave app) so this seems like the way to go, needs to be tested and documented on our side.DONE
Possible improvements
Currently, our CLI uses args for both commands and flags. We may consider changing that to
Commands represent actions, Args are things and Flags are modifiers for those actions.inspired by Cobra. Unfortunately, this would need to be a breaking change.Closes #481