Moving UI Manifests to root manifests directory

[Griffin-Sullivan]

Description

As described in #705 we are moving the UI manifests to the root manifests dir.

How Has This Been Tested?

No testing done yet. Can easily test by checking out the PR and running the script in the clients/ui directory.

Merge criteria:

• All the commits have been signed-off (To pass the DCO check)

• The commits have meaningful messages; the author will squash them after approval or in case of manual merges will ask to merge with squash.
• Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
• The developer has manually tested the changes and verified that the changes work.
• Code changes follow the kubeflow contribution guidelines.

If you have UI changes

• The developer has added tests or explained why testing cannot be added.
• Included any necessary screenshots or gifs if it was a UI change.
• Verify that UI/UX changes conform the UX guidelines for Kubeflow.

[Griffin-Sullivan]

@tarilabs @lucferbux I think you would be the best two to review this!

[tarilabs]

/assing @Al-Pragliola

Alessio, can you kindly dedicate your quick 👀 on this one please?

[Al-Pragliola]

I found 2 small issues in the doc, other than that it looks good to me

[Al-Pragliola]

I tried to follow these steps, but I ran into this error

kubectl apply -k manifests/kustomize/options/ui/overlays/standalone -n kubeflow
serviceaccount/model-registry-ui created
clusterrole.rbac.authorization.k8s.io/model-registry-create-sars created
clusterrole.rbac.authorization.k8s.io/model-registry-retrieve-clusterrolebindings created
clusterrole.rbac.authorization.k8s.io/model-registry-ui-namespaces-reader created
clusterrole.rbac.authorization.k8s.io/model-registry-ui-services-reader created
clusterrolebinding.rbac.authorization.k8s.io/service-access-cluster-binding created
service/model-registry-ui-service created
deployment.apps/model-registry-ui created
Error from server (Invalid): error when creating "manifests/kustomize/options/ui/overlays/standalone": ClusterRoleBinding.rbac.authorization.k8s.io "model-registry-create-sars-binding" is inv
alid: subjects[0].namespace: Required value
Error from server (Invalid): error when creating "manifests/kustomize/options/ui/overlays/standalone": ClusterRoleBinding.rbac.authorization.k8s.io "model-registry-retrieve-clusterrolebinding
s-binding" is invalid: subjects[0].namespace: Required value
Error from server (Invalid): error when creating "manifests/kustomize/options/ui/overlays/standalone": ClusterRoleBinding.rbac.authorization.k8s.io "model-registry-ui-namespaces-reader-bindin
g" is invalid: subjects[0].namespace: Required value
Error from server (Invalid): error when creating "manifests/kustomize/options/ui/overlays/standalone": ClusterRoleBinding.rbac.authorization.k8s.io "model-registry-ui-services-reader-binding"
 is invalid: subjects[0].namespace: Required value

maybe you can use something like this instead

UI_MANIFESTS_PATH=manifests/kustomize/options/ui/overlays/standalone && cd "$UI_MANIFESTS_PATH" && kustomize edit set namespace kubeflow && cd - && kubectl apply -k "$UI_MANIFESTS_PATH"

[Griffin-Sullivan]

oh yeah forgot we changed that. I'm interested what @lucferbux's thoughts are here

[lucferbux]

I like the part that adds the kustomize edit set namespace kubeflow cause that's the right command but I don't think we should parametrize the installation path neither add a new env variable since it won't change in the foreseeable future. But that's a good suggestion.

[Al-Pragliola]

I think this info is outdated, the user should see just 1 pod running

[lucferbux]

Yes, that's right, if we can change it it would be amazing

[lucferbux]

@Griffin-Sullivan can you remove this check from the script?

# Check if the script has rights to push an image into the registry
if ! docker push "${IMG_UI_STANDALONE}" >/dev/null 2>&1; then
  echo -e "\033[31mError: No rights to push the image to the registry ${IMG_UI_STANDALONE}, you can change the image in the env variable IMG_UI_STANDALONE\033[0m"
  exit 1
fi

Maybe leave an echo warning users to have right owners, but it's gonna fail if the user doesn't have a local image, so fist time it's gonna always fail, (it's out of scope I know but it would be nice if we can add it here)

[lucferbux]

Ok, reviewed all the docs and scripts, everything looks great, just the couple of suggestions @Al-Pragliola and I left in the comments and it would be ready to go.

[lucferbux]

/lgtm

🚀

[pboyd]

I walked through the instructions just now without issue. No blocker from my perspective.

[Al-Pragliola]

/lgtm

[tarilabs]

per discussion:

• [Proposal] Review and add UI manifests to the root level manifests directory #705 (comment)

and thank you all for the contributions

/lgtm
/approve

[google-oss-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Al-Pragliola, lucferbux, pboyd, tarilabs

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:

• clients/ui/OWNERS [lucferbux,tarilabs]
• manifests/kustomize/OWNERS [tarilabs]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment