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

Add expand option to support variable expansion #57

Merged
merged 1 commit into from
Sep 19, 2018

Conversation

jheth
Copy link
Contributor

@jheth jheth commented Sep 6, 2018

We are wanting to reference environment variables from existing variables. I've added this as an expand option but could be part of the default behavior if that's desired. This will look for the ${VAR} pattern and replace with the appropriate variable if found.

@ghost
Copy link

ghost commented Sep 6, 2018

There were the following issues with this Pull Request

  • Commit: 267f5e5
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

env_test.go Outdated
ExpandKey string `env:"EXPAND_KEY"`
CompoundKey string `env:"HOST_PORT,expand"`
}
os.Setenv("HOST", "localhost")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was hoping to rely on envDefault for HOST and PORT but the package does not call os.Setenv when parsing the config.

@coveralls
Copy link

coveralls commented Sep 6, 2018

Coverage Status

Coverage increased (+0.02%) to 98.765% when pulling 998eb22 on jheth:add-expand-option into bc122c2 on caarlos0:master.

@jheth jheth force-pushed the add-expand-option branch 2 times, most recently from cd5befe to ebc7b7d Compare September 6, 2018 17:25
@medyagh
Copy link
Collaborator

medyagh commented Sep 10, 2018

@jheth
I wouldn't recommend for Default Behavior for security implication without changing the major version to warn the previous users . but I am okay with having this as an optional feature!

For the sake of readability and usability, I suggest being able to specify the expand option like this,

	type config struct {
		SecretKey   string `env:"SECRET_KEY" ExpandVar:True`
	}

instead of

	type config struct {
		SecretKey   string `env:"SECRET_KEY,expand"`
	}

If you could re implement it that way, I am okay with it be merged.

@caarlos0
Copy link
Owner

caarlos0 commented Sep 11, 2018

We can probably use https://golang.org/pkg/os/#ExpandEnv instead of writing our own function...

@jheth
Copy link
Contributor Author

jheth commented Sep 19, 2018

Thanks @caarlos0 and @medyagh for the suggestions. The simplified the change quite a bit.

env.go Outdated
@@ -114,6 +114,11 @@ func get(field reflect.StructField) (string, error) {
defaultValue := field.Tag.Get("envDefault")
val = getOr(key, defaultValue)

expandVar := field.Tag.Get("ExpandVar")
Copy link
Owner

Choose a reason for hiding this comment

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

I think it should be envExpand to be in the same pattern of the other options.

Adding it to the readme as well would be nice =)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. I've updated and simplified the overall change once again.

@caarlos0
Copy link
Owner

thanks!

@caarlos0 caarlos0 merged commit 5c1feba into caarlos0:master Sep 19, 2018
Copy link
Collaborator

@medyagh medyagh left a comment

Choose a reason for hiding this comment

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

Looks good thanks for the contribution

nexoscp added a commit to nexoscp/env that referenced this pull request Apr 7, 2022
Add expand option to support variable expansion
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.

4 participants