-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Removing google-cloud-core dependency in test_utils package. #3550
Conversation
@dhermes enviornment variables: $ git grep "from google.cloud" test_utils/
test_utils/scripts/run_emulator.py:from google.cloud.environment_vars import BIGTABLE_EMULATOR
test_utils/scripts/run_emulator.py:from google.cloud.environment_vars import GCD_DATASET
test_utils/scripts/run_emulator.py:from google.cloud.environment_vars import GCD_HOST
test_utils/scripts/run_emulator.py:from google.cloud.environment_vars import PUBSUB_EMULATOR |
Ha. Lame. I'm going to leave this un-merged for at least 12 hours. I want to see if @lukesneeringer has anything to chime in with. |
weird that test_utils is its own package. Couldn't we just roll it into google-cloud-core? |
I disagree. It's weird to put testing helpers in code that users have installed on their machines. |
I mostly agree, but lesser of two evils - google-cloud-core is an "infrastructure" package (like gax), so I am not morally opposed to it having helpers for both creating packages and tests for those packages. |
FWIW, We actually to have |
@tseaver Yes I'm aware, and would love to see that module either removed (due to better usage of |
I did this originally and @dhermes made me change it. I would still prefer that. |
So this discussion shouldn't block the "bad dep" we currently have in the source tree. Shall I file an issue to move the discussion and then merge this issue? |
If removing the dep does not break anything, I have no objection. |
d367f7e
to
27360c9
Compare
SGTM @lukesneeringer, just amended the commit so that it's just a removal. Fingers crossed. |
Aw man, no packages to test:
|
I'm just going to merge this and if there is carnage we can roll it back ASAP. |
@lukesneeringer Why is this a dependency in the
test_utils
package?