-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
fix make build; set IAM policy following read/modify/write #2751
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.
I would also change the Makefile from
build-bootstrap: generate fmt vet
to
build-bootstrap: /tmp/v2 deepcopy generate fmt vet
otherwise lgtm
/lgtm |
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.
Reviewed 3 of 10 files at r1.
Reviewable status: 3 of 10 files reviewed, 3 unresolved discussions (waiting on @abhi-g, @ellis-bigelow, @gabrielwen, and @kunmingg)
bootstrap/cmd/bootstrap/app/gcpUtils.go, line 145 at r1 (raw file):
// Clear existing bindings for auto-generated service accounts of current deployment. // Those bindings could be leftover from previous actions. func ClearServiceAccountpolicy(currentPolicy *cloudresourcemanager.Policy, req ApplyIamRequest) {
nit: ClearServiceAccountPolicy
bootstrap/cmd/bootstrap/app/gcpUtils.go, line 147 at r1 (raw file):
func ClearServiceAccountpolicy(currentPolicy *cloudresourcemanager.Policy, req ApplyIamRequest) { serviceAccounts := map[string]bool{ fmt.Sprintf("serviceAccount:%v-admin@%v.iam.gserviceaccount.com", req.Cluster, req.Project): true,
move this function to here and use it?
kubeflow/bootstrap/pkg/kfapp/gcp/gcp.go
Line 126 in b066e07
func getSA(name string, nameSuffix string, project string) string { |
bootstrap/cmd/bootstrap/app/gcpUtils_test.go, line 45 at r1 (raw file):
}, }, Etag: "NeedPreserve",
- why adding this field?
- could we use enum or consts instead of random appeared strings?
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.
Reviewable status: 3 of 11 files reviewed, 2 unresolved discussions (waiting on @abhi-g, @ellis-bigelow, @gabrielwen, and @kunmingg)
bootstrap/cmd/bootstrap/app/gcpUtils.go, line 145 at r1 (raw file):
Previously, gabrielwen (Hung-Ting Wen) wrote…
nit: ClearServiceAccountPolicy
Done
bootstrap/cmd/bootstrap/app/gcpUtils.go, line 147 at r1 (raw file):
Previously, gabrielwen (Hung-Ting Wen) wrote…
move this function to here and use it?
kubeflow/bootstrap/pkg/kfapp/gcp/gcp.go
Line 126 in b066e07
func getSA(name string, nameSuffix string, project string) string {
Will do in followup RPs
bootstrap/cmd/bootstrap/app/gcpUtils_test.go, line 45 at r1 (raw file):
Previously, gabrielwen (Hung-Ting Wen) wrote…
- why adding this field?
- could we use enum or consts instead of random appeared strings?
Adding it to test to make sure we won't accidentally delete Etag.
Updated.
updated |
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kunmingg 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 |
…2751) * fix make build; set IAM policy following read/modify/write * address comments
…2751) * fix make build; set IAM policy following read/modify/write * address comments
This change is