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

Use envconfig fork to abstract os package #2337

Merged
merged 2 commits into from
Jan 26, 2022
Merged

Use envconfig fork to abstract os package #2337

merged 2 commits into from
Jan 26, 2022

Conversation

mstoykov
Copy link
Contributor

Previous to this the envconfig package directly called os.LookupEnv.
With this it also takes a function that can replace this call giving us
the ability to not use for example map[string]string for the backing
instead of having to touch the global environment in tests.

@mstoykov
Copy link
Contributor Author

I didn't use replace and instead changed the module path in the fork because:

  1. I preferred it as it looks cleaner
  2. but also i figured out later that otherwise this will break xk6 as replace clauses are only taken from the final module - if one of our dependency has raplace that won't have any effect, we will need to add the same replace in order for it to work.

Luckily, the changes for the most part are pretty small ... so that isn't really a problem.

The two other problems with envconfig seem to be more hairy so at least for now I don't plan on fixing them, but I am taking PRs :P

@na-- na-- added this to the v0.37.0 milestone Jan 24, 2022
Base automatically changed from lintCmd to master January 25, 2022 14:02
Previous to this the envconfig package directly called os.LookupEnv.
With this it also takes a function that can replace this call giving us
the ability to not use for example map[string]string for the backing
instead of having to touch the global environment in tests.
@mstoykov
Copy link
Contributor Author

This is the actual envconfig change for which it was forked. It's backwards compatible on purpose, but we can change that if we decide in the future. The only other change is renaming the module.

Copy link
Member

@na-- na-- left a comment

Choose a reason for hiding this comment

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

LGTM! One step closer... ☺️

@yorugac
Copy link
Contributor

yorugac commented Jan 25, 2022

@mstoykov test-tip failed here: https://github.com/grafana/k6/runs/4938790631 ? That's not the flaky one it seems 😕

@mstoykov
Copy link
Contributor Author

That's not the flaky one it seems

It is, it just fails really rarely.

@mstoykov mstoykov merged commit 7fa42e1 into master Jan 26, 2022
@mstoykov mstoykov deleted the forkEnvConfig branch January 26, 2022 08:37
@na-- na-- added the breaking change for PRs that need to be mentioned in the breaking changes section of the release notes label Mar 3, 2022
@olegbespalov olegbespalov removed the breaking change for PRs that need to be mentioned in the breaking changes section of the release notes label Mar 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants