Figure out projectID for GCS automatically from credentials.json #5029
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5029 +/- ##
==========================================
- Coverage 62.48% 62.44% -0.04%
==========================================
Files 192 192
Lines 27756 27781 +25
==========================================
+ Hits 17342 17348 +6
- Misses 9180 9197 +17
- Partials 1234 1236 +2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #5029 +/- ##
==========================================
- Coverage 61.29% 61.23% -0.06%
==========================================
Files 192 192
Lines 28368 28391 +23
==========================================
- Hits 17387 17386 -1
- Misses 9734 9754 +20
- Partials 1247 1251 +4
Continue to review full report at Codecov.
|
pkg/http/listener_test.go
Outdated
| @@ -778,12 +777,14 @@ func TestHTTPListenerAcceptParallel(t *testing.T) { | |||
| if err != nil { | |||
| t.Fatalf("Test %d: accept: expected = <nil>, got = %v", i+1, err) | |||
| } | |||
There was a problem hiding this comment.
Don't do these changes in your PR @krishnasrinivas - you should send a separate fix.
harshavardhana
requested review from
balamurugana
and removed request for
donatello
krishnasrinivas
force-pushed
the
figure-out-projectid
branch
from
30ae003
to
f3cbd12
Compare
cmd/gateway-gcs.go
Outdated
| @@ -248,11 +252,36 @@ type gcsGateway struct { | |||
|
|
|||
| const googleStorageEndpoint = "storage.googleapis.com" | |||
|
|
|||
| // Returns projectID from the GOOGLE_APPLICATION_CREDENTIALS file. | |||
| func gcsParseProjectID(path string) (projectID string, err error) { | |||
There was a problem hiding this comment.
credsJSONFile would be more clear than path
There was a problem hiding this comment.
I have renamed it to credsFile
| errorIf(errGCSProjectIDNotFound, "project-id should be provided as argument or GOOGLE_APPLICATION_CREDENTIALS should be set with path to credentials.json") | ||
| cli.ShowCommandHelpAndExit(ctx, "gcs", 1) | ||
| } | ||
| if projectID != "" && !isValidGCSProjectIDFormat(projectID) { |
There was a problem hiding this comment.
As project ID from command line has higher precedence over env var GOOGLE_APPLICATION_CREDENTIALS. I think below if block would be clear
credsJSONFile := os.Getenv("GOOGLE_APPLICATION_CREDENTIALS")
if projectID == "" {
if credsJSONFIle == "" {
fatalIf(...)
}
if projectID, err = gcsParseProjectID(credsJSONFile); err != nil {
fatalIf(err, ...)
}
}This way newGCSGateway() could use projectID argument without knowing about env var GOOGLE_APPLICATION_CREDENTIALS
There was a problem hiding this comment.
I tried to do this @balamurugana but our source code structure does not allow this flow, it needs a refactor to allow it.
i.e I tried to make the flow such that newGCSGateway() does not access GOOGLE_APPLICATION_CREDENTIALS but it was not possible. It needs a separate refactor of the functions in gateway-main.go
There was a problem hiding this comment.
OK. Probably we need to open an issue?
krishnasrinivas
force-pushed
the
figure-out-projectid
branch
from
f3cbd12
to
4719b69
Compare
krishnasrinivas
force-pushed
the
figure-out-projectid
branch
from
4719b69
to
e355dc0
Compare
fixes #5027
Description
We parse projectID from credentials.json file, hence it need not be passed in the command line if GOOGLE_APPLICATION_CREDENTIALS is set to the path of credentials.json
How Has This Been Tested?
manual, unit testing
Types of changes
Checklist: