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.

Sign up for GitHub

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

Jump to bottom

Add BFF dev mode for local K8s testing #560

Merged
merged 1 commit into from
Nov 18, 2024
Merged

Add BFF dev mode for local K8s testing #560

merged 1 commit into from
Nov 18, 2024

Conversation

Griffin-Sullivan
Copy link
Contributor

@Griffin-Sullivan Griffin-Sullivan commented Nov 12, 2024 •
edited
Loading

Description

This PR allows developers to run a local MR cluster in kind and port forward their service to work with the BFF running locally.

How Has This Been Tested?

Follow the dev guide all the way to step 4.

Port forward with:

kubectl port-forward svc/model-registry-service 8081:8080 -n kubeflow

Run the BFF with:

make run DEV_MODE=true DEV_MODE_PORT=8081

and the UI in a separate terminal.

You should see an empty MR that you can interact with (make models, versions, archive, restore, etc)

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.

lucferbux
lucferbux suggested changes Nov 13, 2024
View reviewed changes
Copy link
Contributor

@lucferbux lucferbux left a comment

Choose a reason for hiding this comment

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

Everything looks great, only nit would be parametrization of ports.

clients/ui/bff/internal/api/middleware.go
if err != nil {
return "", err
}

if app.config.DevMode {
serviceDetails.ClusterIP = "localhost"
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to bring the internal conversation here, it would be really interesting to parametrize the port too, that way we can get whatever port users might have used for port-forwarding.

lucferbux
lucferbux suggested changes Nov 14, 2024
View reviewed changes
Copy link
Contributor

@lucferbux lucferbux left a comment

Choose a reason for hiding this comment

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

Tested and works great, only last nit: can you expand the docs to reflect this new logic?

@google-oss-prow google-oss-prow bot added size/M and removed size/S labels Nov 15, 2024
alexcreasy
alexcreasy suggested changes Nov 15, 2024
View reviewed changes
Copy link
Contributor

@alexcreasy alexcreasy left a comment

Choose a reason for hiding this comment

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

Looks good, just one thing to take a look at!

clients/ui/bff/internal/api/middleware.go Outdated
@@ -83,11 +84,17 @@ func resolveBearerToken(k8s integrations.KubernetesClientInterface, header http.
return bearerToken, nil
}

func resolveModelRegistryURL(id string, client integrations.KubernetesClientInterface) (string, error) {
serviceDetails, err := client.GetServiceDetailsByName(id)
func resolveModelRegistryURL(id string, app *App) (string, error) {
Copy link
Contributor

@alexcreasy alexcreasy Nov 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not a fan of passing a pointer to the app struct into this function. It's meant as a utility function so I would stick to value semantics and add params for each value it requires.

An alternative would be to convert this function into a method of the app struct and use a receiver param to access what you need.

I'm happy with either of the above.

Copy link
Member

Choose a reason for hiding this comment

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

Agree with Alex here.

@Griffin-Sullivan
Add BFF dev mode for local K8s testing
c9921e8 
Signed-off-by: Griffin-Sullivan <gsulliva@redhat.com>
ederign
ederign suggested changes Nov 15, 2024
View reviewed changes
Copy link
Member

@ederign ederign left a comment

Choose a reason for hiding this comment

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

just to a second review after my comment above ^

lucferbux
lucferbux approved these changes Nov 18, 2024
View reviewed changes
Copy link
Contributor

@lucferbux lucferbux left a comment

Choose a reason for hiding this comment

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

/lgtm

Excelent work here!

@google-oss-prow Google OSS Prow
Copy link

@lucferbux: changing LGTM is restricted to collaborators

In response to this:

/lgtm

Excelent work 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.

@alexcreasy
Copy link
Contributor

/lgtm

@google-oss-prow google-oss-prow bot added the lgtm label Nov 18, 2024
alexcreasy
alexcreasy approved these changes Nov 18, 2024
View reviewed changes
Copy link
Contributor

@alexcreasy alexcreasy left a comment

Choose a reason for hiding this comment

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

Great work, thanks Griffin!

@google-oss-prow Google OSS Prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexcreasy, lucferbux

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-prow google-oss-prow bot merged commit b82c9e4 into kubeflow:main Nov 18, 2024
16 checks passed
@tarilabs tarilabs mentioned this pull request Nov 20, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ederign ederign ederign requested changes

@alexcreasy alexcreasy alexcreasy approved these changes

@lucferbux lucferbux lucferbux approved these changes

@andreyvelich andreyvelich Awaiting requested review from andreyvelich

Assignees

@alexcreasy alexcreasy

Labels
approved Area/UI lgtm size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Griffin-Sullivan @alexcreasy @ederign @lucferbux