-
Notifications
You must be signed in to change notification settings - Fork 464
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
feat: adding support for bq #1878
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @bradmiro
infra/blueprint-test/pkg/bq/bq.go
Outdated
} | ||
|
||
// ActivateCredsAndEnvVars activates credentials and exports auth related envvars. | ||
func ActivateCredsAndEnvVars(t testing.TB, creds string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not seem BQ specific and might be a better fit in utils?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copied this directly from gcloud.go
since auth is the same - I can move it from here (and there) to utils if that makes sense. Or I can leave it as-is in gcloud.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, since the logic is the same is there any advantage to mirroring this in bq package? Maybe we can just leave it in gcloud for now.
infra/blueprint-test/pkg/bq/bq.go
Outdated
} | ||
|
||
// stringFromTextAndArgs convert msg and args to formatted text | ||
func stringFromTextAndArgs(msgAndArgs ...interface{}) string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are starting to use this for both gcloud and bq, I think centralizing this in utils might be good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can move this out of both files but that might result in a breaking change if the function in gcloud.go
was referenced in any downstream projects - wdyt? (I'm less familiar with Golang best practices so apologies if there's an easy fix here I'm unaware of)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it was unexported from gcloud package it is okay to move
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
/gcbrun |
/gcbrun |
/gcbrun |
/gcbrun |
/gcbrun |
infra/blueprint-test/pkg/bq/bq.go
Outdated
} | ||
|
||
// ActivateCredsAndEnvVars activates credentials and exports auth related envvars. | ||
func ActivateCredsAndEnvVars(t testing.TB, creds string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, since the logic is the same is there any advantage to mirroring this in bq package? Maybe we can just leave it in gcloud for now.
d4b0385
into
GoogleCloudPlatform:master
Adding support for
bq
command line tool.