-
-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. |
||
return err | ||
} | ||
} | ||
|
@@ -54,7 +54,7 @@ func unmarshalStruct(data interface{}, prefix, tag string) error { | |
} | ||
|
||
// unmarshalField handles unmarshaling individual fields based on tags | ||
func unmarshalField(field reflect.Value, tag string, prefix string) error { | ||
func unmarshalField(field reflect.Value, tag string, prefix string, structPtr interface{}) error { | ||
tagOpts := parseTag(tag) | ||
value, found := findFieldValue(tagOpts.keys, prefix) | ||
|
||
|
@@ -67,10 +67,14 @@ func unmarshalField(field reflect.Value, tag string, prefix string) error { | |
found = true | ||
} | ||
|
||
if !found { | ||
if !found && tagOpts.fallback != "" { | ||
value = tagOpts.fallback | ||
} | ||
|
||
if tagOpts.expand { | ||
value = expandVariables(value, structPtr) | ||
} | ||
|
||
if tagOpts.required && value == "" { | ||
return fmt.Errorf("required environment variable %s is not set", tagOpts.keys[0]) | ||
} | ||
|
@@ -82,6 +86,58 @@ func unmarshalField(field reflect.Value, tag string, prefix string) error { | |
return nil | ||
} | ||
|
||
// 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Good find, I've moved the |
||
matches := re.FindAllStringSubmatch(value, -1) | ||
|
||
for _, match := range matches { | ||
var envVar string | ||
if match[1] != "" { | ||
envVar = match[1] // ${var} syntax | ||
} else { | ||
envVar = match[2] // $var syntax | ||
} | ||
|
||
envValue, ok := Lookup(envVar) // Lookup the environment variable; use default if not set | ||
if !ok { | ||
envValue = getDefaultFromStruct(envVar, structPtr) | ||
} | ||
value = strings.Replace(value, match[0], envValue, -1) | ||
} | ||
|
||
return value | ||
} | ||
|
||
// getDefaultFromStruct retrieves the default value from the struct if available | ||
func getDefaultFromStruct(fieldName string, structPtr interface{}) string { | ||
v := reflect.ValueOf(structPtr).Elem() | ||
t := v.Type() | ||
|
||
for i := 0; i < v.NumField(); i++ { | ||
fieldType := t.Field(i) | ||
tag := fieldType.Tag.Get("env") | ||
tagOpts := parseTag(tag) | ||
|
||
if tagOpts.keys[0] == fieldName { | ||
if tagOpts.fallback != "" { | ||
return tagOpts.fallback | ||
} | ||
} | ||
// Handle nested structs | ||
if fieldType.Type.Kind() == reflect.Struct { | ||
nestedStructPtr := v.Field(i).Addr().Interface() | ||
nestedValue := getDefaultFromStruct(fieldName, nestedStructPtr) | ||
if nestedValue != "" { | ||
return nestedValue | ||
} | ||
} | ||
} | ||
|
||
return "" | ||
} | ||
|
||
// Helper function to read file content | ||
func readFileContent(filePath string) (string, error) { | ||
content, err := os.ReadFile(filePath) | ||
|
@@ -108,6 +164,7 @@ type tagOptions struct { | |
fallback string | ||
required bool | ||
file bool | ||
expand bool | ||
} | ||
|
||
// parseTag parses the struct tag into tagOptions | ||
|
@@ -117,6 +174,7 @@ func parseTag(tag string) tagOptions { | |
var fallbackValue string | ||
required := false | ||
file := false | ||
expand := false | ||
|
||
if len(parts) > 1 { | ||
extraParts := parts[1] | ||
|
@@ -132,23 +190,24 @@ func parseTag(tag string) tagOptions { | |
if !inBrackets { | ||
part := extraParts[start:i] | ||
start = i + 1 | ||
parsePart(part, &fallbackValue, &required, &file) | ||
parsePart(part, &fallbackValue, &required, &file, &expand) | ||
} | ||
} | ||
} | ||
part := extraParts[start:] | ||
parsePart(part, &fallbackValue, &required, &file) | ||
parsePart(part, &fallbackValue, &required, &file, &expand) | ||
} | ||
|
||
return tagOptions{ | ||
keys: keys, | ||
fallback: fallbackValue, | ||
required: required, | ||
file: file, | ||
expand: expand, | ||
} | ||
} | ||
|
||
func parsePart(part string, fallbackValue *string, required *bool, file *bool) { | ||
func parsePart(part string, fallbackValue *string, required *bool, file *bool, expand *bool) { | ||
if strings.Contains(part, "default=[") || strings.Contains(part, "fallback=[") { | ||
re := regexp.MustCompile(`(?:default|fallback)=\[(.*?)]`) | ||
matches := re.FindStringSubmatch(part) | ||
|
@@ -165,6 +224,8 @@ func parsePart(part string, fallbackValue *string, required *bool, file *bool) { | |
*required = true | ||
} else if strings.TrimSpace(part) == "file" { | ||
*file = true | ||
} else if strings.TrimSpace(part) == "expand" { | ||
*expand = true | ||
} | ||
} | ||
|
||
|
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
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:
In bash, you do define them in order. Since variables are expected to be defined before you start your code, IE:
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