Update base MR service to include labels and annotations for the BFF #525

Griffin-Sullivan · 2024-10-30T13:18:35Z

Description

Adding a new label called component and setting it to model-registry. This will allow us to filter services in the BFF rather than looping through all services.

Also adding annotations for displayName and description. The BFF looks for these annotations to display the name of the MR when changing registries in the UI. Everything will work without these but this is a nicer UX and can help with determining which model registry is which.

How Has This Been Tested?

Tested locally on kind and got:

curl localhost:4000/api/v1/model_registry
{
	"data": [
		{
			"name": "model-registry-service",
			"displayName": "Default Model Registry",
			"description": "The default model registry"
		}
	]
}

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.

ederign

Looks good to me, but let's wait for other chime in before merge.

lucferbux

/lgtm

Let's wait on @tarilabs just to confirm.

google-oss-prow · 2024-11-04T09:22:09Z

@lucferbux: changing LGTM is restricted to collaborators

In response to this:

/lgtm

Let's wait on @tarilabs just to confirm.

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.

tarilabs

thanks @Griffin-Sullivan

/lgtm

some minor comments and tagged people for 👀

tarilabs · 2024-11-04T13:16:20Z

manifests/kustomize/base/model-registry-service.yaml

    app: metadata
+    component: model-registry


My suggestion is to seize this chance to align label app and component in the Service here as:

Suggested change

app: metadata

component: model-registry

app: model-registry

component: model-registry-server

to be consistent with the Deployment:

model-registry/manifests/kustomize/base/model-registry-deployment.yaml

Lines 2 to 6 in 709a2db

kind: Deployment

metadata:

name: model-registry-deployment

labels:

component: model-registry-server

@Al-Pragliola @isinyaaa @dhirajsb wdyt ?

The app name is used to group related resources in a single app. E.g. the MR itself, it's deployment, service, istio resources, etc. should all use the same app name.
So, we set it to the MR name downstream. The downstream labels are:

labels: app: modelregistry-sample app.kubernetes.io/component: model-registry app.kubernetes.io/created-by: model-registry-operator app.kubernetes.io/instance: modelregistry-sample app.kubernetes.io/managed-by: model-registry-operator app.kubernetes.io/name: modelregistry-sample app.kubernetes.io/part-of: model-registry component: model-registry name: modelregistry-sample

So I should add all of these labels?

@Al-Pragliola can you take up the task of consolidating the useful ones from here into a replacements yaml for an MR template for overlays?

@Griffin-Sullivan we can start with app, and component labels, we can definitely skip the operator label values as they don't apply.
We should use app.kubernetes.io labels to be consistent with k8s recommendations.

@dhirajsb let me know what you think I added the changes.

lgtm @Griffin-Sullivan

Let's keep the service name the same. The sample I shared was to show the label names as an example, not the label values for kubeflow manifests.

tarilabs · 2024-11-04T13:17:12Z

manifests/kustomize/base/model-registry-service.yaml

+    displayName: Default Model Registry
+    description: The default model registry


Suggested change

displayName: Default Model Registry

description: The default model registry

displayName: Kubeflow Model Registry

description: An example model registry

lucferbux · 2024-11-11T08:01:23Z

/lgtm
@tarilabs @dhirajsb what do you think know?

google-oss-prow · 2024-11-11T08:01:26Z

@lucferbux: changing LGTM is restricted to collaborators

In response to this:

/lgtm
@tarilabs @dhirajsb what do you think know?

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.

tarilabs · 2024-11-11T08:50:07Z

/lgtm @tarilabs @dhirajsb what do you think know?

it looks to me #525 (comment) is not yet addressed.

dhirajsb · 2024-11-11T18:11:22Z

/lgtm

google-oss-prow · 2024-11-11T18:11:27Z

@dhirajsb: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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.

dhirajsb · 2024-11-11T18:20:42Z

manifests/kustomize/base/model-registry-service.yaml

@@ -2,8 +2,16 @@ kind: Service
 apiVersion: v1
 metadata:
  labels:
-    app: metadata
-  name: model-registry-service
+    app: modelregistry-sample


Suggested change

app: modelregistry-sample

app: model-registry-service

dhirajsb · 2024-11-11T18:21:11Z

manifests/kustomize/base/model-registry-service.yaml

-  name: model-registry-service
+    app: modelregistry-sample
+    app.kubernetes.io/component: model-registry
+    app.kubernetes.io/instance: modelregistry-sample


Suggested change

app.kubernetes.io/instance: modelregistry-sample

app.kubernetes.io/instance: model-registry-service

dhirajsb · 2024-11-11T18:21:27Z

manifests/kustomize/base/model-registry-service.yaml

+    app: modelregistry-sample
+    app.kubernetes.io/component: model-registry
+    app.kubernetes.io/instance: modelregistry-sample
+    app.kubernetes.io/name: modelregistry-sample


Suggested change

app.kubernetes.io/name: modelregistry-sample

app.kubernetes.io/name: model-registry-service

dhirajsb · 2024-11-11T18:21:55Z

manifests/kustomize/base/model-registry-service.yaml

+  annotations:
+    displayName: Kubeflow Model Registry
+    description: An example model registry
+  name: modelregistry-sample


Suggested change

name: modelregistry-sample

name: model-registry-service

Signed-off-by: Griffin-Sullivan <gsulliva@redhat.com>

dhirajsb

/lgtm

google-oss-prow · 2024-11-13T15:26:15Z

@dhirajsb: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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.

tarilabs

/lgtm
/approve

google-oss-prow · 2024-11-18T08:44:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhirajsb, ederign, lucferbux, 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:

~~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

google-oss-prow bot requested review from tarilabs and Tomcli October 30, 2024 13:18

github-actions bot added the Area/Manifests label Oct 30, 2024

google-oss-prow bot added the size/XS label Oct 30, 2024

ederign approved these changes Oct 30, 2024

View reviewed changes

lucferbux approved these changes Nov 4, 2024

View reviewed changes

tarilabs reviewed Nov 4, 2024

View reviewed changes

google-oss-prow bot assigned tarilabs Nov 4, 2024

google-oss-prow bot added the lgtm label Nov 4, 2024

Griffin-Sullivan force-pushed the service-update branch from b89e628 to d35b934 Compare November 4, 2024 20:55

google-oss-prow bot removed the lgtm label Nov 4, 2024

Griffin-Sullivan force-pushed the service-update branch from d35b934 to a846a62 Compare November 11, 2024 13:55

google-oss-prow bot added size/S and removed size/XS labels Nov 11, 2024

dhirajsb suggested changes Nov 11, 2024

View reviewed changes

Update base MR service to include labels and annotations for the BFF

e87f6f4

Signed-off-by: Griffin-Sullivan <gsulliva@redhat.com>

Griffin-Sullivan force-pushed the service-update branch from a846a62 to e87f6f4 Compare November 12, 2024 18:04

dhirajsb approved these changes Nov 13, 2024

View reviewed changes

tarilabs approved these changes Nov 18, 2024

View reviewed changes

google-oss-prow bot added the lgtm label Nov 18, 2024

google-oss-prow bot added the approved label Nov 18, 2024

google-oss-prow bot merged commit ee461ba into kubeflow:main Nov 18, 2024
16 checks passed

tarilabs mentioned this pull request Nov 18, 2024

[pull] main from kubeflow:main opendatahub-io/model-registry#145

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update base MR service to include labels and annotations for the BFF #525

Update base MR service to include labels and annotations for the BFF #525

Griffin-Sullivan commented Oct 30, 2024 •

edited

Loading

ederign left a comment

lucferbux left a comment

google-oss-prow bot commented Nov 4, 2024

tarilabs left a comment

tarilabs Nov 4, 2024

dhirajsb Nov 5, 2024

Griffin-Sullivan Nov 6, 2024

dhirajsb Nov 6, 2024

dhirajsb Nov 6, 2024

Griffin-Sullivan Nov 11, 2024

dhirajsb Nov 11, 2024

dhirajsb Nov 11, 2024

tarilabs Nov 4, 2024

lucferbux commented Nov 11, 2024

google-oss-prow bot commented Nov 11, 2024

tarilabs commented Nov 11, 2024

dhirajsb commented Nov 11, 2024

google-oss-prow bot commented Nov 11, 2024

dhirajsb Nov 11, 2024

dhirajsb Nov 11, 2024

dhirajsb Nov 11, 2024

dhirajsb Nov 11, 2024

dhirajsb left a comment

google-oss-prow bot commented Nov 13, 2024

tarilabs left a comment

google-oss-prow bot commented Nov 18, 2024

	kind: Deployment
	metadata:
	name: model-registry-deployment
	labels:
	component: model-registry-server

		displayName: Default Model Registry
		description: The default model registry

	app.kubernetes.io/instance: modelregistry-sample
	app.kubernetes.io/instance: model-registry-service

	app.kubernetes.io/name: modelregistry-sample
	app.kubernetes.io/name: model-registry-service

Update base MR service to include labels and annotations for the BFF #525

Update base MR service to include labels and annotations for the BFF #525

Conversation

Griffin-Sullivan commented Oct 30, 2024 • edited Loading

Description

How Has This Been Tested?

Merge criteria:

ederign left a comment

Choose a reason for hiding this comment

lucferbux left a comment

Choose a reason for hiding this comment

google-oss-prow bot commented Nov 4, 2024

tarilabs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucferbux commented Nov 11, 2024

google-oss-prow bot commented Nov 11, 2024

tarilabs commented Nov 11, 2024

dhirajsb commented Nov 11, 2024

google-oss-prow bot commented Nov 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhirajsb left a comment

Choose a reason for hiding this comment

google-oss-prow bot commented Nov 13, 2024

tarilabs left a comment

Choose a reason for hiding this comment

google-oss-prow bot commented Nov 18, 2024

Griffin-Sullivan commented Oct 30, 2024 •

edited

Loading