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

feat(bff): update bff service filtering to match model registry services by component: model-registry label #633

Merged
merged 1 commit into from
Dec 13, 2024

Conversation

ederign
Copy link
Member

@ederign ederign commented Dec 10, 2024

Description

This PR sits on top of #630 and update bff service filtering to match model registry services by component: model-registry label.

In this specific PR: (commit)

  • clients/ui/bff/README.md : add a FAQ section detailing what is setup on env test mock and also how we do model registry service filtering
    On clients/ui/bff/internal/integrations/k8s.go
  • Removed some duplicated logic on GetServiceNames()
  • On GetServiceDetails, removed component filtering in favor of label filtering (much more efficient)
  • I've also make all context with similar values (30seconds)
  • clients/ui/bff/internal/mocks/k8s_mock.go: added label for our 3 services and created a new one without the label.

As illustration, I've I remove the label filtering:

curl -i -H "kubeflow-userid: [email protected]" localhost:4000/api/v1/model_registry                                                                         (kind-kubeflow/default)
HTTP/1.1 200 OK
Access-Control-Allow-Origin: *
Content-Type: application/json
Date: Tue, 10 Dec 2024 15:10:16 GMT
Content-Length: 556

{
	"data": [
		{
			"name": "model-registry-dora",
			"displayName": "Model Registry Dora",
			"description": "Model Registry Dora description"
		},
		{
			"name": "model-registry",
			"displayName": "Model Registry",
			"description": "Model Registry Description"
		},
		{
			"name": "model-registry-bella",
			"displayName": "Model Registry Bella",
			"description": "Model Registry Bella description"
		},
		{
			"name": "non-model-registry", <----- WRONG
			"displayName": "Not a Model Registry",
			"description": "Not a Model Registry Bella description"
		}
	]
}

With filtering:

curl -i -H "kubeflow-userid: [email protected]" localhost:4000/api/v1/model_registry                                                                         (kind-kubeflow/default)
HTTP/1.1 200 OK
Access-Control-Allow-Origin: *
Content-Type: application/json
Date: Tue, 10 Dec 2024 15:31:35 GMT
Content-Length: 413

{
	"data": [
		{
			"name": "model-registry-dora",
			"displayName": "Model Registry Dora",
			"description": "Model Registry Dora description"
		},
		{
			"name": "model-registry",
			"displayName": "Model Registry",
			"description": "Model Registry Description"
		},
		{
			"name": "model-registry-bella",
			"displayName": "Model Registry Bella",
			"description": "Model Registry Bella description"
		}
	]
}
```

## How Has This Been Tested? 
Basic sanity check of the app

## Merge criteria:
<!--- This PR will be merged by any repository approver when it meets all the points in the checklist -->
- All the commits have been [_signed-off_](https://github.com/kubeflow/community/tree/master/dco-signoff-hook#signing-off-commits)  (To pass the `DCO` check)

<!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
- [X] The commits have meaningful messages; the author will squash them after approval or in case of manual merges will ask to merge with squash.
- [X] Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
- [X] The developer has manually tested the changes and verified that the changes work.
- [X] Code changes follow the [kubeflow contribution guidelines](https://www.kubeflow.org/docs/about/contributing/).
 

@ederign
Copy link
Member Author

ederign commented Dec 10, 2024

As soon as you are able to review and (if it's all right) merge #630, I'll rebase this PR

…ces by component: model-registry label

Signed-off-by: Eder Ignatowicz <[email protected]>
@ederign ederign force-pushed the fix-service-filtering branch from b8be495 to 6caec82 Compare December 11, 2024 13:33
@google-oss-prow google-oss-prow bot added size/L and removed size/XL labels Dec 11, 2024
Copy link
Contributor

@Griffin-Sullivan Griffin-Sullivan left a comment

Choose a reason for hiding this comment

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

Testing the mock mode and everything works. Need someone else to do the live cluster testing.

var httpPort int32
hasHTTPPort := false
for _, port := range service.Spec.Ports {
if port.Name == "http-api" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it safe to assume the MR team won't change this port name?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess so, otherwise it would need to change the contract of the service.


displayName := ""
description := ""
if service.Spec.ClusterIP == "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is code from before, but is there a case where this would ever happen when deploying MR?

Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Griffin-Sullivan

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

@alexcreasy
Copy link
Contributor

/lgtm

@google-oss-prow google-oss-prow bot added the lgtm label Dec 13, 2024
@google-oss-prow google-oss-prow bot merged commit 8dd89af into kubeflow:main Dec 13, 2024
16 checks passed
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.

3 participants