-
Notifications
You must be signed in to change notification settings - Fork 893
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 ml pipeline access from kfp step #2795
Fix ml pipeline access from kfp step #2795
Conversation
…yaml Signed-off-by: Krzysztof Romanowski <krzysztof.romanowski.kr1@roche.com>
…esn't have auth header Signed-off-by: Krzysztof Romanowski <krzysztof.romanowski.kr1@roche.com>
Thank you for the PR, I think the changes here should also trigger https://github.com/kubeflow/manifests/blob/master/.github/workflows/pipeline_run_from_notebook.yaml. |
Signed-off-by: juliusvonkohout <45896133+juliusvonkohout@users.noreply.github.com>
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: juliusvonkohout 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 |
@@ -90,11 +90,14 @@ jobs: | |||
|
|||
while True: | |||
status = client.get_run(run_id=run_id).state | |||
if status not in ["SUCCEEDED", "FAILED", "ERROR"]: | |||
if status in ["PENDING", "RUNNING"]: |
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.
@kromanow94 a bit late to request this change, but could we not have the python code directly on the GH action but convert to a .py
file, and use that from the action?
This will allow users to also run the tests locally if they want
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 think it makes a lot of sense. Do you mind if I do it next week?
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.
just want to say I also vote for moving to a .py
file.
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 think this is a part of a wider topic to reorganize tests. I created an issue for that:
* fail gh action if pipeline failed in .github/workflows/pipeline_test.yaml Signed-off-by: Krzysztof Romanowski <krzysztof.romanowski.kr1@roche.com> * allow access to ml-pipeline when using trusted requestPrincipal or doesn't have auth header Signed-off-by: Krzysztof Romanowski <krzysztof.romanowski.kr1@roche.com> * add more triggers for the workflow Signed-off-by: juliusvonkohout <45896133+juliusvonkohout@users.noreply.github.com> --------- Signed-off-by: Krzysztof Romanowski <krzysztof.romanowski.kr1@roche.com> Signed-off-by: juliusvonkohout <45896133+juliusvonkohout@users.noreply.github.com> Co-authored-by: Krzysztof Romanowski <krzysztof.romanowski.kr1@roche.com> Co-authored-by: juliusvonkohout <45896133+juliusvonkohout@users.noreply.github.com> Signed-off-by: hansinikarunarathne <107214435+hansinikarunarathne@users.noreply.github.com>
Fix ml pipeline access from kfp step
✏️ A brief description of the changes
I changed the:
.github/workflows/pipeline_test.yaml
to fail if the KF Pipeline Run has failed,AuthorizationPolicy/ml-pipeline
to allow access for both scenarios:kubeflow-userid
header to allow access from KFP Steps📦 List any dependencies that are required for this change
N/A
🐛 If this PR is related to an issue, please put the link to the issue here.
#2794
✅ Contributor checklist
DCO
check)cla/google
check)@kimwnasptd , @juliusvonkohout