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.

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

Add kustomization overlay: katib-openshift #1513

Merged

Conversation

maanur
Copy link
Contributor

@maanur maanur commented Apr 8, 2021

What this PR does / why we need it:
Current version of Katib have some issues and inconveniences when deployed to OpenShift, so I tried to introduce a workaround in form of kustomization overlay.

Workaround to #1512, but only for OpenShift 4.4+

Special notes for your reviewer:
Tested against OpenShift 4.6.21

Release note:

NONE

@google-cla
Copy link

google-cla bot commented Apr 8, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@aws-kf-ci-bot
Copy link
Contributor

Hi @maanur. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@maanur
Copy link
Contributor Author

maanur commented Apr 8, 2021

@googlebot I signed it!

@google-cla
Copy link

google-cla bot commented Apr 8, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@maanur
Copy link
Contributor Author

maanur commented Apr 8, 2021

@googlebot I signed it!

Copy link
Member

@andreyvelich andreyvelich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this effort, this is very helpful!
I think we should wait until we merge this PR: #1498, since we are making some changes to store Katib manifests.
After that, we can rebase this PR and merge it.

/cc @yanniszark @davidspek @johnugeorge @gaocegege

@davidspek
Copy link
Contributor

@andreyvelich I'll work on my PR tomorrow, hopefully it will then be ready to merge. If after that any help is needed with this PR I'm happy to provide assistance.

@andreyvelich
Copy link
Member

Thank you for your help @davidspek!

@andreyvelich
Copy link
Member

@maanur I apologies for the delays on this PR.
Please, can you rebase and we can merge it ?

@andreyvelich
Copy link
Member

/ok-to-test

@davidspek
Copy link
Contributor

/test kubeflow-katib-presubmit

Copy link
Contributor

@davidspek davidspek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added some comment to help migrate this PR to implement the changes introduced in #1498. After these changes this PR should be good to go.

@maanur maanur changed the title Add kustomization overlay: katib-standalone-openshift Add kustomization overlay: katib-openshift Apr 13, 2021
@maanur maanur force-pushed the kustomize/katib-standalone-openshift branch from 81a6f90 to 7d71691 Compare April 13, 2021 19:01
@maanur maanur force-pushed the kustomize/katib-standalone-openshift branch from 7d71691 to 83678f1 Compare April 13, 2021 19:02
@maanur
Copy link
Contributor Author

maanur commented Apr 13, 2021

Manually tested against OpenShift version 4.6.21

@davidspek
Copy link
Contributor

@maanur Looks good to me.
/lgtm

Copy link
Member

@andreyvelich andreyvelich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this effort @maanur!
I left few comments from my side.

It would be great if you could also left your review @yanniszark @nakfour @gaocegege @johnugeorge

@andreyvelich
Copy link
Member

Thank you for the changes @maanur.
/lgtm
/hold for the review from others.

@davidspek
Copy link
Contributor

/lgtm

Copy link
Member

@andreyvelich andreyvelich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can merge it.
Thank you for this effort @maanur!
/hold cancel
/approve

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andreyvelich, maanur

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

@google-oss-robot google-oss-robot merged commit 54854c1 into kubeflow:master Apr 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants