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

gocqlxtest: get config from env #248

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Ivan-Feofanov
Copy link

PR for the issue #247
With the replacement builtin flags package to github.com/namsral/flag we could take config as from flags so from env variables

@Ivan-Feofanov Ivan-Feofanov changed the title gocqltest: get config from env gocqlxtest: get config from env Jan 30, 2023
Copy link

@Gor027 Gor027 left a comment

Choose a reason for hiding this comment

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

@avelanarius LGTM. Can you merge it?

@zimnx
Copy link
Collaborator

zimnx commented Feb 15, 2023

@Gor027 I don't think such little feature requires additional dependency (tagged as pre) to entire project. Keeping low subset of dependencies in libraries is a good thing. I'm against merging this.

@Gor027
Copy link

Gor027 commented Feb 15, 2023

@zimnx I definitely agree with you, however, as it aims to be used for testing purposes only, perhaps it will not hurt to add it. I am not so familiar with Go, maybe there is a way to add a dependency for tests (or examples, or benchmarks) only, like [dev-dependencies] in Rust.

@zimnx
Copy link
Collaborator

zimnx commented Feb 15, 2023

Dependencies in Go are per module. Gocqlx consist of single module, so even test dependencies are part of library dependencies. This PR changes go.sum and go.mod of main module, so they are going into library dependencies.

@mmatczuk
Copy link
Contributor

It's not worth adding another dependency for all users.

Having said that the Gocqlx core module could add some automation around env vars but it needs to be explicit (well defined function) and not use 3rd party libraries.

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.

None yet

4 participants