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

Bind env variables with ENV-BINDER, clearing tests #676

Merged
merged 2 commits into from Oct 25, 2021
Merged

Bind env variables with ENV-BINDER, clearing tests #676

merged 2 commits into from Oct 25, 2021

Conversation

kuritka
Copy link
Collaborator

@kuritka kuritka commented Oct 21, 2021

Thanks to AbsaOSS/ENV-BINDER we have moved the responsibility for bindings of environment variables to an external, well tested package.

ENV-BINDER works with unexported fields, so we can bind the values of some fields and calculate a new value from them in the calculation phase. Such example is enum, where env_variable has the value "info" but the internal state of the structure has the value 1. (I have also created a PR in kelsyhightower/envconfig, which solves the problem with private fields, unfortunately it seems that nobody takes care of the library for a long time).

Thanks to the delegation of responsibility I could delete about a third of the test without changing the depressolver
coverage, which increased to 95% with the changes.

closes #683

Signed-off-by: kuritka kuritka@gmail.com

@netlify
Copy link

netlify bot commented Oct 21, 2021

✔️ Deploy Preview for k8gb-preview ready!

🔨 Explore the source changes: 39a25b3

🔍 Inspect the deploy log: https://app.netlify.com/sites/k8gb-preview/deploys/61766a641e109c0007e016bd

😎 Browse the preview: https://deploy-preview-676--k8gb-preview.netlify.app

@kuritka kuritka changed the title Bind env variables with ENV-BINDER, clean tests Bind env variables with ENV-BINDER, clearing tests Oct 21, 2021
k0da
k0da previously approved these changes Oct 22, 2021
@@ -80,62 +80,70 @@ const (
type Log struct {
// Level [panic, fatal, error,warn,info,debug,trace], defines level of logger, default: info
Level zerolog.Level
// Format [simple,json] specifies how the logger prints values
// Format [simple,json] specifies how the logger prints values; 2=simple
Copy link
Member

Choose a reason for hiding this comment

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

what does 2=simple mean?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess should be default=simple?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was forgotten because I first tried to bind an integer directly to an enum (it works, but it's terrible for the user).

comment modified, changes amended

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In terms of how we set the variables in the helm, nothing has changed at all. It's fully compatible

// Username
Username string
Username string `env:"EXTERNAL_DNS_INFOBLOX_WAPI_USERNAME"`
Copy link
Member

Choose a reason for hiding this comment

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

Just a note for another PR, we need to get rid of EXTERNAL_DNS part in this env var name as technically it is not backed by external-dns anymore

HTTPRequestTimeout int
// HTTPPoolConnections seconds; default = 10
HTTPPoolConnections int
Password string `env:"EXTERNAL_DNS_INFOBLOX_WAPI_PASSWORD"`
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

Thanks to ENV-BINDER we have moved the responsibility for bindings of environment variables to an external, well tested package.

ENV-BINDER works with unexported fields, so we can bind the values of some fields and calculate a new value from them in the calculation phase.
An example is enum, where env_variable has the value "info" but the internal state of the structure has the value 1. (I have also created a
PR in [kelsyhightower/envconfig](kelseyhightower/envconfig#198), which solves the problem with private fields).

Thanks to the delegation of responsibility I could delete about a third of the test without changing the depressolver
 coverage, which increased to 95% with the changes.

Signed-off-by: kuritka <kuritka@gmail.com>
…ord (#688)

Signed-off-by: Jimmy <speckmaier58@gmail.com>
Signed-off-by: kuritka <kuritka@gmail.com>
Copy link
Member

@ytsarev ytsarev left a comment

Choose a reason for hiding this comment

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

lgtm, great stuff

@kuritka kuritka merged commit b188498 into master Oct 25, 2021
@kuritka kuritka deleted the env2 branch October 25, 2021 10:04
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.

Get rid of EXTERNAL_DNS_ prefix at Infoblox ENV vars
5 participants