-
-
Notifications
You must be signed in to change notification settings - Fork 1
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
Allow variable expansion #9
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 4 4
Lines 289 325 +36
=========================================
+ Hits 289 325 +36 ☔ View full report in Codecov by Sentry. |
Port string `env:"PORT,default=8080"` | ||
Address string `env:"ADDRESS,default=${HOST}:$PORT,expand"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should use different name here than host and port, because the example might let people think HOST and PORT already need to be provided as fields
So maybe, something like
Address string `env:"ADDRESS,default=${HOST}:$PORT,expand"` | |
Address string `env:"ADDRESS,default=${ADDR}:$BIND,expand"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pretty standard variable expansion behavior, take Bash for example:
HOST="localhost"
PORT="8080"
ADDR="${HOST}:${PORT}"
echo $ADDR
In bash, you do define them in order. Since variables are expected to be defined before you start your code, IE:
HOST=localhost PORT=8080 go run main.go
The behavior of expand
would work with that.
The reverse lookup of default values also allows for the code to be able to leverage those, so if values aren't set in the environment, you don't have to run into errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point about the default value for variable used in another variable using expand.
It should be covered by a dedicated test maybe
Setting just
PORT="8081"
and expect to have
type Config struct {
Debug bool `env:"DEBUG"`
Host string `env:"HOST,default=localhost"`
Port string `env:"PORT,default=8080"`
Address string `env:"ADDRESS,default=${HOST}:$PORT,expand"`
}
cfg := Config{
Host: "localhost"
Port: "8081"
Address: "localhost:8081"
}
@@ -36,7 +36,7 @@ func unmarshalWithPrefix(data interface{}, prefix string) error { | |||
continue | |||
} | |||
|
|||
if err := unmarshalField(field, tag, prefix); err != nil { | |||
if err := unmarshalField(field, tag, prefix, data); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's getting too complicated, it was OK when you had 2 fields
you should define a struct with all the fields you need, then create a variable from this struct, then pass a reference to the variable and, that's it
No more code rewrites when adding an new feature, and also less pointers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There wasn't really a way to make the default value lookup work without this. I'm open to suggestions, but variable expansion is a strong feature of environment variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could make a suggestion, but I have to get back from holidays, as I only have a phone with me right now.
So, I'm opening a refactoring issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// expandVariables replaces placeholders with actual environment variable values or defaults if not set. | ||
func expandVariables(value string, structPtr interface{}) string { | ||
// Handle both ${var} and $var syntax | ||
re := regexp.MustCompile(`\$\{([^}]+)\}|\$([A-Za-z_][A-Za-z0-9_]*)`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This regexp will be computed each time you call the method, it uses a lot of resources
Move it out the method, especially because you are using must compile. It would panic quickly if you update the regexp to something invalid, not when this feature is used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good find, I've moved the MustCompiles
to variables in main
but will wait to do a release until we've got through any other feedback.
expand