Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fixes 'kfctl creates invalid IAM policy binding file if email not set' #2718

Merged
merged 1 commit into from Mar 21, 2019
Merged

fixes 'kfctl creates invalid IAM policy binding file if email not set' #2718

merged 1 commit into from Mar 21, 2019

Conversation

kkasravi
Copy link
Contributor

@kkasravi kkasravi commented Mar 16, 2019 •
edited

fixes #2707

This change is Reviewable

@kkasravi kkasravi mentioned this pull request Mar 16, 2019
Merged
@kkasravi
Copy link
Contributor Author

/assign @gabrielwen

@kkasravi
Copy link
Contributor Author

/assign @jlewi

@jlewi jlewi changed the title fixes 'kfctl creates invalid IAM policy binding file if email not set' [wip] fixes 'kfctl creates invalid IAM policy binding file if email not set' Mar 17, 2019
@jlewi
Copy link
Contributor

jlewi commented Mar 17, 2019

Marking WIP into blocking PRs are merged.

@kkasravi kkasravi force-pushed the kfctl_emailfix branch 4 times, most recently from 3e76031 to 627be71 Compare March 20, 2019 15:54
@kkasravi kkasravi changed the title [wip] fixes 'kfctl creates invalid IAM policy binding file if email not set' fixes 'kfctl creates invalid IAM policy binding file if email not set' Mar 20, 2019
@kkasravi
Copy link
Contributor Author

@gabrielwen this fix may be a bit of a hack - running gcloud to get the account if it's not provided. You may have a better way.

@gabrielwen
Copy link
Contributor

@gabrielwen this fix may be a bit of a hack - running gcloud to get the account if it's not provided. You may have a better way.

Looks fine to me. It's just if it were me, I wouldn't depend on shell out as much as possible. gcloud config get-value reads from ~/.config/gcloud and returns the value we need. The benefit of using shell out, though, is that gcloud already takes care of finding files and parsing information for us.

@gabrielwen
Copy link
Contributor

/lgtm

@kkasravi
Copy link
Contributor Author

/hold cancel

@kkasravi
Copy link
Contributor Author

@kunmingg @lluunn this is ready to merge, please approve

@lluunn
Copy link
Contributor

lluunn commented Mar 21, 2019

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lluunn

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 0c30416 into kubeflow:master Mar 21, 2019
@kkasravi kkasravi deleted the kfctl_emailfix branch April 2, 2019 17:49
saffaalvi pushed a commit to StatCan/kubeflow that referenced this pull request Feb 11, 2021
@kkasravi @k8s-ci-robot
fixes 'kfctl creates invalid IAM policy binding file if email not set' (
4080fdf 
kubeflow#2718)
saffaalvi pushed a commit to StatCan/kubeflow that referenced this pull request Feb 12, 2021
@kkasravi @k8s-ci-robot
fixes 'kfctl creates invalid IAM policy binding file if email not set' (
bf09137 
kubeflow#2718)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ellistarn ellistarn Awaiting requested review from ellistarn

@gaocegege gaocegege Awaiting requested review from gaocegege

Assignees

@jlewi jlewi

@gabrielwen gabrielwen

Labels
approved cla: yes lgtm size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

kfctl creates invalid IAM policy binding file if email not set
6 participants
@kkasravi @jlewi @gabrielwen @lluunn @k8s-ci-robot @googlebot