Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: read config from environment variables #1608

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vichle
Copy link

@vichle vichle commented May 13, 2024

This is a first draft of reading configuration from environment variables (#561) . I chose a different approach than #1041, let me know what you think! I've only added support for a single arg for now: backend using the FAKE_GCS_BACKEND environment variable. I'll update the PR with the rest of the arguments if you think this is a good way of doing it.

I don't usually write golang, so please be thorough in your feedback :)

Open questions:

  • Is this a good approach?
  • Should all arguments be supported?
  • Do we need more tests?

@vichle vichle changed the title read config from environment variables WIP: read config from environment variables May 15, 2024
@vichle vichle marked this pull request as ready for review May 15, 2024 05:26
@vichle
Copy link
Author

vichle commented May 31, 2024

@fsouza some feedback would be appreciated :)

@fsouza
Copy link
Owner

fsouza commented May 31, 2024

@fsouza some feedback would be appreciated :)

Sorry I completely missed this, will take a look later today!

@Isakdl
Copy link

Isakdl commented Sep 5, 2024

@fsouza any love for this? :) Would be great to have support for envs for github actions services.

Copy link
Owner

@fsouza fsouza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for such a bad turnaround.

I think this is fine as it behaves as expected: flag has precedence, but environment variable can be used to override the default value.

If you want to handle all args that'd be appreciated. We need a solution for flags that are not strings, but we don't need to make it too generic given we only have to handle strings and uint.

I'm comfortable with the basic testing you have and maybe exhaustive testing for envVarOrDefault.

Thanks for contributing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants