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

Fix ml-pipeline-ui doesn't have gcp permission bug #594

Merged
merged 3 commits into from
Nov 2, 2019
Merged

Fix ml-pipeline-ui doesn't have gcp permission bug #594

merged 3 commits into from
Nov 2, 2019

Conversation

Bobgy
Copy link
Contributor

@Bobgy Bobgy commented Oct 31, 2019 •
edited
Loading

Which issue is resolved by this Pull Request:
Related to kubeflow/pipelines#2501

Description of your changes:
Mount gcp service account credentials to ml-pipeline-ui.

Checklist:

  • Unit tests have been rebuilt:
    1. cd manifests/tests
    2. make generate
    3. make test

This change is Reviewable

@jlewi
Copy link
Contributor

jlewi commented Oct 31, 2019

Is this intended to be a temporary fix or a long term fix?

See kubeflow/pipelines#2501 (comment)

@jlewi
Copy link
Contributor

jlewi commented Oct 31, 2019

@Bobgy I think relying on GCP secrets is fine as a quick fix but we should file a follow on bug to switch to workload identity before the next release.

@k8s-ci-robot k8s-ci-robot added size/L and removed size/S labels Nov 1, 2019
@Bobgy
Copy link
Contributor Author

Bobgy commented Nov 1, 2019

It seems my generated tests don't match current format. Is that a gofmt version mismatch?

@Bobgy
Copy link
Contributor Author

Bobgy commented Nov 1, 2019

I tried to use go1.12.4, but it doesn't change anything.

Besides, the tests are failing. I couldn't really understand what the error message means. Can you help me with that?

@Bobgy Bobgy mentioned this pull request Nov 1, 2019
Closed
@Bobgy
Copy link
Contributor Author

Bobgy commented Nov 1, 2019

This will be a quick fix. For long term fix, tracked in kubeflow/pipelines#1691 (comment)

@jlewi
Copy link
Contributor

jlewi commented Nov 1, 2019

@Bobgy The E2E tests are failing because of an issue with GCE. See: kubeflow/kfctl/issues/69

Please note that after this fix is submitted we will need to cherry pick the PR onto the v0.7-branch.

We have a shell script to easily create cherry picks but you can also do it manually
https://github.com/kubeflow/kubeflow/blob/master/hack/cherry-picks.md

@jlewi
Copy link
Contributor

jlewi commented Nov 1, 2019

/lgtm
/approve

@jlewi
Copy link
Contributor

jlewi commented Nov 1, 2019

/test all

@jlewi
Copy link
Contributor

jlewi commented Nov 1, 2019

@Bobgy the test failed because of an issue that's been fixed on master.
Could you please rebase on the latest master?

@k8s-ci-robot k8s-ci-robot removed the lgtm label Nov 2, 2019
@jlewi
Copy link
Contributor

jlewi commented Nov 2, 2019

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jlewi

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 eb328ec into kubeflow:master Nov 2, 2019
@Bobgy Bobgy deleted the patch-2 branch November 2, 2019 02:54
Bobgy added a commit to Bobgy/manifests that referenced this pull request Nov 2, 2019
@Bobgy
Fix ml-pipeline-ui doesn't have gcp permission bug (kubeflow#594)
9202961 
* Fix ml-pipeline-ui doesn't have gcp permission bug

* Moved patch to gcp overlay

* Regenerated tests
@Bobgy Bobgy mentioned this pull request Nov 2, 2019
Merged
k8s-ci-robot pushed a commit that referenced this pull request Nov 2, 2019
@Bobgy @k8s-ci-robot
Fix ml-pipeline-ui doesn't have gcp permission bug (#594) (#602)
317c0d8 
* Fix ml-pipeline-ui doesn't have gcp permission bug

* Moved patch to gcp overlay

* Regenerated tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jlewi jlewi jlewi left review comments

@krishnadurai krishnadurai Awaiting requested review from krishnadurai

Assignees

@jlewi jlewi

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

Successfully merging this pull request may close these issues.
4 participants
@Bobgy @jlewi @k8s-ci-robot @googlebot