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

Standardized testdata config filepath pattern #6480

Closed
rmfitzpatrick opened this issue Dec 1, 2021 · 5 comments · Fixed by #6980
Closed

Standardized testdata config filepath pattern #6480

rmfitzpatrick opened this issue Dec 1, 2021 · 5 comments · Fixed by #6980
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@rmfitzpatrick
Copy link
Contributor

rmfitzpatrick commented Dec 1, 2021

Is your feature request related to a problem? Please describe.
As mentioned in #6459 (comment), there is a current test pattern for specifying config files of the form:

cfg, err := configtest.LoadConfigAndValidate(path.Join(".", "testdata", "config.yaml"), factories)

This invocation is slightly problematic for two reasons:

  1. the initial "." is unnecessary.
  2. path.Join() hardcodes the separator as "/" instead of picking it up from the os like filepath.Join() does, which is used by the underlying koanf loader.

Describe the solution you'd like
Replace all path.Join(".", with filepath.Join(

Describe alternatives you've considered
Using embed could be preferable to using paths and loaders overall since the underlying byte slice would be available in the packages. I have superficially confirmed that embedded items in test functions don't make their way to the final build using hexdump, but a more rigorous assessment would be necessary. Since the existing helper works with paths and uses* the config map loaders, requiring embed directives as replacements and using bytes directly is probably overkill for most cases.

@jpkrohling
Copy link
Member

Your proposed solution seems simple and effective enough.

@jpkrohling jpkrohling added enhancement New feature or request good first issue Good for newcomers labels Dec 2, 2021
@DiptoChakrabarty
Copy link
Contributor

@jpkrohling @rmfitzpatrick I would like to work on this.

@jpkrohling
Copy link
Member

Go for it, @DiptoChakrabarty

@bogdandrutu
Copy link
Member

@jpkrohling can you track to ensure we do the same in the core repo?

@jpkrohling
Copy link
Member

@codeboten beat me to it, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants