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

Manifests for the new UI #1476

Merged

Conversation

kimwnasptd
Copy link
Member

@kimwnasptd kimwnasptd commented Mar 17, 2021

What this PR does / why we need it:
This PR adds manifests for the new UI #1421 we've been working on in 1.3. These manifests are aimed to make it easier for people to try out this UI and give us some feedback. The default UI will be the current one.

Which issue(s) this PR fixes:
First step for #1469 and #1474

Special notes for your reviewer:

  1. The manifests are copy paste of the current UI's manifests, with a modification of the image.
  2. I specifically kept the names of the deployment/svc the same so that people can apply these yamls over the current UI and vice versa. If you don't like this approach I can use the katib-new-ui names for these yamls instead, but this might complicate things because we will need to create another VirtualService for ISTIO
  3. The image I set to the deployment is docker.io/kubeflowkatib/katib-new-ui

Just some questions I had:

  1. Should I also create a small README.md in the new-ui folder to iterate again that this UI is not yet on par with the current one?
  2. What is the difference between the manifests/v1beta1/components and manifests/v1beta1 folders?

/cc @andreyvelich @gaocegege @johnugeorge

@kimwnasptd
Copy link
Member Author

I see that the Argo workflow for the presubmit tests fail in the task setup-katib(0) because:

...
katib-db-manager-685694f86f-x48dd   0/1     CrashLoopBackOff   7          21m

https://argo.kubeflow-testing.com/workflows/kubeflow-test-infra/kubeflow-katib-presubmit-e2e-v1beta1-1476-ede0c2c-0624-6704?tab=workflow

I'll try to rerun the tests in case this was ephemeral
/retest

@andreyvelich
Copy link
Member

@kimwnasptd Thank you for doing this.
Can you rebase, please ?

@andreyvelich
Copy link
Member

Should I also create a small README.md in the new-ui folder to iterate again that this UI is not yet on par with the current one?

Yes, that would be great. We can say how user should modify installs to use the new UI.

@@ -0,0 +1,38 @@
apiVersion: apps/v1
Copy link
Member

Choose a reason for hiding this comment

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

Please can you check the latest manifest for the UI: https://github.com/kubeflow/katib/tree/master/manifests/v1beta1/components/ui.
We've made few changes to to the labels and imagePullPolicy

@kimwnasptd
Copy link
Member Author

@andreyvelich as we briefly discussed since these manifests are identical with the one for the current UI instead of copy pasting the almost same manifests I'll edit the README of the repo to instruct users on how to use the new UI.

I'll use this PR for this, but I'll force push the changes instead

Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>
@kimwnasptd kimwnasptd force-pushed the feature-kimwnasptd-new-ui-manifests branch from ede0c2c to e08e969 Compare March 18, 2021 20:40
@kimwnasptd
Copy link
Member Author

Force pushed the new small section in the README, @andreyvelich @gaocegege @johnugeorge looking forward to your feedback!

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 the update @kimwnasptd.
I added comments.

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Adds a top level table-of-contents entry for the new UI as well.

Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>
Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>
@kimwnasptd kimwnasptd force-pushed the feature-kimwnasptd-new-ui-manifests branch from 24a9839 to 06303c3 Compare March 18, 2021 22:42
README.md Outdated Show resolved Hide resolved
@andreyvelich
Copy link
Member

Overall text looks good, thank you @kimwnasptd.
What do you think @gaocegege @johnugeorge ?

Signed-off-by: Kimonas Sotirchos <kimwnasptd@arrikto.com>
@andreyvelich
Copy link
Member

/lgtm
/hold for the review from @gaocegege and @johnugeorge

Copy link
Member

@gaocegege gaocegege left a comment

Choose a reason for hiding this comment

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

/lgtm

Thanks for your contribution! 🎉 👍

@andreyvelich
Copy link
Member

Let's merge this.
/hold cancel

@andreyvelich
Copy link
Member

/approve

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

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

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 86c6cfb into kubeflow:master Mar 19, 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.

4 participants