Call gcloud auth activate-service-account if GOOGLE_APPLICATION_CREDENTIALS is set
#1757
Conversation
googlebot
added
the
cla: yes
tekton-robot
added
the
size/M
| @@ -26,7 +26,14 @@ import ( | |||
| corev1 "k8s.io/api/core/v1" | |||
| ) | |||
|
|
|||
| func Test_Invalid_NewStorageResource(t *testing.T) { | |||
| const activateServiceAccountScript = `#!/usr/bin/env bash | |||
There was a problem hiding this comment.
NB: This is copied here because the other version is not exported (and shouldn't be), and tests are in package v1alpha1_test.
Is there a preferred way to maintain these invariants together?
There was a problem hiding this comment.
Should this be in an external package ? (as it's solely related to gcs, maybe it should be in the apis package at all ?
There was a problem hiding this comment.
I'd like it not to be exported anywhere at all. But I guess if I have to, a separate package is better.
At least when resources are pluggable all this will leave this repo, right?
There was a problem hiding this comment.
@imjasonh that's kinda my thought yeah 👼
There was a problem hiding this comment.
+1 for separate package. Seems like the best option for now!
There was a problem hiding this comment.
In the end I decided to make the resource add a single step with a script that activates the SA then calls gsutil cp/gsutil rsync, and to just put the full details of the script into each test.
When this gets extensible-ized this will all get cleaner tests will be split out with the code.
|
/lgtm |
| @@ -226,14 +261,14 @@ func Test_GetInputSteps(t *testing.T) { | |||
| if tc.wantErr && err == nil { | |||
| t.Fatalf("Expected error to be %t but got %v:", tc.wantErr, err) | |||
| } | |||
| if d := cmp.Diff(gotSpec.GetStepsToPrepend(), tc.wantSteps); d != "" { | |||
| t.Errorf("Error mismatch between download containers spec: %s", d) | |||
| if d := cmp.Diff(tc.wantSteps, gotSpec.GetStepsToPrepend()); d != "" { | |||
| @@ -26,7 +26,14 @@ import ( | |||
| corev1 "k8s.io/api/core/v1" | |||
| ) | |||
|
|
|||
| func Test_Invalid_NewStorageResource(t *testing.T) { | |||
| const activateServiceAccountScript = `#!/usr/bin/env bash | |||
There was a problem hiding this comment.
+1 for separate package. Seems like the best option for now!
tekton-robot
added
the
approved
imjasonh
force-pushed
the
activate-sa
branch
from
a404746 to
f7a1234
Compare
tekton-robot
added
size/L
imjasonh
force-pushed
the
activate-sa
branch
3 times, most recently
from
902e539 to
a5b2cd5
Compare
…DENTIALS is set This used to be done by the gsutil image. When that image was replaced by the vanilla google/cloud-sdk image, this documented behavior broke. By adding this to the step added by the resource, the behavior is maintained without needing to maintain our own image to perform activation.
imjasonh
force-pushed
the
activate-sa
branch
from
a5b2cd5 to
85020ca
Compare
| @@ -50,7 +51,7 @@ require ( | |||
| golang.org/x/sync v0.0.0-20190911185100-cd5d95a43a6e // indirect | |||
| golang.org/x/sys v0.0.0-20191110163157-d32e6e3b99c4 // indirect | |||
| golang.org/x/time v0.0.0-20191024005414-555d28b269f0 // indirect | |||
| google.golang.org/api v0.10.0 // indirect | |||
| google.golang.org/api v0.10.0 | |||
There was a problem hiding this comment.
why is this not indirect anymore ? same for cloud.google.com/go/storage above, I don't see any imports in our code for those, shouldn't this mean they should be indirect ?
There was a problem hiding this comment.
I have no idea. I just build and commit whatever Go thinks is right. 🤷♂️
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dibyom, vdemeester The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
This used to be done by our own gsutil image. When that image was replaced
by the vanilla
google/cloud-sdkimage, this documented behavior broke.By adding this as a step added by the resource, the behavior is
maintained without needing to maintain our own image to perform
activation.
Fixes #1748
/hold needs e2e test
cc @dustym
Changes
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Double check this list of stuff that's easy to miss:
cmddir, please updatethe release Task to build and release this image.
Reviewer Notes
If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.
Release Notes