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

fix: Remove leading underscore #113

Merged
merged 1 commit into from
Dec 14, 2022
Merged

fix: Remove leading underscore #113

merged 1 commit into from
Dec 14, 2022

Conversation

arbourd
Copy link
Contributor

@arbourd arbourd commented Dec 12, 2022

go-envconfig made a change in v0.8.x where any leading underscore (ex: _APP_NAME) is invalid and will break env var parsing.

This commit removes the leading prefixed underscore from built-in options, and sets the default prefix string to VK_.

All new prefixes must end with an underscore when this change is released and adopted. For example, an app that used _MY_ENV for an env var and PREFIX for the prefix, but be changed to PREFIX_ for the prefix and MY_ENV for the env var.

@arbourd arbourd force-pushed the remove-leading-underscore branch from 91cb8b3 to 6a9b37d Compare December 12, 2022 22:54
@cohix
Copy link
Collaborator

cohix commented Dec 13, 2022

@arbourd in the finalize method, could you add a check to ensure that any provided prefix includes a trailing underscore, and add it automatically if it doesn't?

go-envconfig made a change in v0.8.x where any leading underscore is
invalid and will break env var parsing. This commit removes the leading
prefixed underscore from built-ins.

All new prefixes must end with an underscore when this change is
released and adopted.
@arbourd arbourd force-pushed the remove-leading-underscore branch from 6a9b37d to bb50308 Compare December 13, 2022 20:01
Comment on lines +79 to +83
// Append trailing _ if prefix is missing one
if !strings.HasSuffix(prefix, "_") {
prefix = prefix + "_"
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could describe this in the docs but I think we'd want people to use the prefix APP_ over APP, where this fallback exists for backwards compatibility reasons.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nah we'll be sunsetting vektor eventually so no point in doing anything special. This is good.

vk/test/routes_test.go Show resolved Hide resolved
@arbourd arbourd merged commit 39117f9 into main Dec 14, 2022
@arbourd arbourd deleted the remove-leading-underscore branch December 14, 2022 15:16
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.

2 participants