-
Notifications
You must be signed in to change notification settings - Fork 43
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
Adds ability to serve mocked data for MR API calls via cli flag #377
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,8 @@ import ( | |
|
||
type Envelope map[string]interface{} | ||
|
||
type TypedEnvelope[T any] map[string]T | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @alexcreasy in a next iteration shall we remove the untyped one? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ederign yes, I'd like to, the only reason I didn't just change it was for speed right now :) |
||
|
||
func (app *App) WriteJSON(w http.ResponseWriter, status int, data any, headers http.Header) error { | ||
|
||
js, err := json.MarshalIndent(data, "", "\t") | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,135 @@ | ||
package api | ||
|
||
import ( | ||
"bytes" | ||
"context" | ||
"encoding/json" | ||
"github.com/kubeflow/model-registry/pkg/openapi" | ||
"github.com/kubeflow/model-registry/ui/bff/internals/mocks" | ||
"github.com/stretchr/testify/assert" | ||
"io" | ||
"net/http" | ||
"net/http/httptest" | ||
"testing" | ||
) | ||
|
||
func TestGetRegisteredModelHandler(t *testing.T) { | ||
mockMRClient, _ := mocks.NewModelRegistryClient(nil) | ||
mockClient := new(mocks.MockHTTPClient) | ||
|
||
testApp := App{ | ||
modelRegistryClient: mockMRClient, | ||
} | ||
|
||
req, err := http.NewRequest(http.MethodGet, | ||
"/api/v1/model-registry/model-registry/registered_models/1", nil) | ||
assert.NoError(t, err) | ||
|
||
ctx := context.WithValue(req.Context(), httpClientKey, mockClient) | ||
req = req.WithContext(ctx) | ||
|
||
rr := httptest.NewRecorder() | ||
|
||
testApp.GetRegisteredModelHandler(rr, req, nil) | ||
rs := rr.Result() | ||
|
||
defer rs.Body.Close() | ||
|
||
body, err := io.ReadAll(rs.Body) | ||
assert.NoError(t, err) | ||
var registeredModelRes TypedEnvelope[openapi.RegisteredModel] | ||
err = json.Unmarshal(body, ®isteredModelRes) | ||
assert.NoError(t, err) | ||
|
||
assert.Equal(t, http.StatusOK, rr.Code) | ||
|
||
var expected = TypedEnvelope[openapi.RegisteredModel]{ | ||
"registered_model": mocks.GetRegisteredModelMocks()[0], | ||
} | ||
|
||
//TODO assert the full structure, I couldn't get unmarshalling to work for the full customProperties values | ||
// this issue is in the test only | ||
assert.Equal(t, expected["registered_model"].Name, registeredModelRes["registered_model"].Name) | ||
} | ||
|
||
func TestGetAllRegisteredModelsHandler(t *testing.T) { | ||
mockMRClient, _ := mocks.NewModelRegistryClient(nil) | ||
mockClient := new(mocks.MockHTTPClient) | ||
|
||
testApp := App{ | ||
modelRegistryClient: mockMRClient, | ||
} | ||
|
||
req, err := http.NewRequest(http.MethodGet, | ||
"/api/v1/model-registry/model-registry/registered_models", nil) | ||
assert.NoError(t, err) | ||
|
||
ctx := context.WithValue(req.Context(), httpClientKey, mockClient) | ||
req = req.WithContext(ctx) | ||
|
||
rr := httptest.NewRecorder() | ||
|
||
testApp.GetAllRegisteredModelsHandler(rr, req, nil) | ||
rs := rr.Result() | ||
|
||
defer rs.Body.Close() | ||
|
||
body, err := io.ReadAll(rs.Body) | ||
assert.NoError(t, err) | ||
var registeredModelsListRes TypedEnvelope[openapi.RegisteredModelList] | ||
err = json.Unmarshal(body, ®isteredModelsListRes) | ||
assert.NoError(t, err) | ||
|
||
assert.Equal(t, http.StatusOK, rr.Code) | ||
|
||
var expected = TypedEnvelope[openapi.RegisteredModelList]{ | ||
"registered_model_list": mocks.GetRegisteredModelListMock(), | ||
} | ||
|
||
assert.Equal(t, expected["registered_model_list"].Size, registeredModelsListRes["registered_model_list"].Size) | ||
assert.Equal(t, expected["registered_model_list"].PageSize, registeredModelsListRes["registered_model_list"].PageSize) | ||
assert.Equal(t, expected["registered_model_list"].NextPageToken, registeredModelsListRes["registered_model_list"].NextPageToken) | ||
assert.Equal(t, len(expected["registered_model_list"].Items), len(registeredModelsListRes["registered_model_list"].Items)) | ||
} | ||
|
||
func TestCreateRegisteredModelHandler(t *testing.T) { | ||
mockMRClient, _ := mocks.NewModelRegistryClient(nil) | ||
mockClient := new(mocks.MockHTTPClient) | ||
|
||
testApp := App{ | ||
modelRegistryClient: mockMRClient, | ||
} | ||
|
||
newModel := openapi.NewRegisteredModelCreate("Model One") | ||
newModelJSON, err := newModel.MarshalJSON() | ||
assert.NoError(t, err) | ||
|
||
reqBody := bytes.NewReader(newModelJSON) | ||
|
||
req, err := http.NewRequest(http.MethodPost, | ||
"/api/v1/model-registry/model-registry/registered_models", reqBody) | ||
assert.NoError(t, err) | ||
|
||
ctx := context.WithValue(req.Context(), httpClientKey, mockClient) | ||
req = req.WithContext(ctx) | ||
|
||
rr := httptest.NewRecorder() | ||
|
||
testApp.CreateRegisteredModelHandler(rr, req, nil) | ||
rs := rr.Result() | ||
|
||
defer rs.Body.Close() | ||
|
||
body, err := io.ReadAll(rs.Body) | ||
assert.NoError(t, err) | ||
var registeredModelRes openapi.RegisteredModel | ||
err = json.Unmarshal(body, ®isteredModelRes) | ||
assert.NoError(t, err) | ||
|
||
assert.Equal(t, http.StatusCreated, rr.Code) | ||
|
||
var expected = mocks.GetRegisteredModelMocks()[0] | ||
|
||
assert.Equal(t, expected.Name, registeredModelRes.Name) | ||
assert.NotEmpty(t, rs.Header.Get("location")) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,4 +3,5 @@ package config | |
type EnvConfig struct { | ||
Port int | ||
MockK8Client bool | ||
MockMRClient bool | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
package data | ||
|
||
import ( | ||
"log/slog" | ||
) | ||
|
||
type ModelRegistryClientInterface interface { | ||
RegisteredModelInterface | ||
} | ||
|
||
type ModelRegistryClient struct { | ||
logger *slog.Logger | ||
RegisteredModel | ||
} | ||
|
||
func NewModelRegistryClient(logger *slog.Logger) (ModelRegistryClientInterface, error) { | ||
return &ModelRegistryClient{logger: logger}, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a review, it's more of a question, are we gonna get 8000 as the default port? We have 4000 in the frontend for the api, if so, I'll do a follow up PR to change the references there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, the default port seems to be PORT ?= 4000 (on Makefile)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But on those instructions, I just to show how to do with an alternative one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes... if you leave out the PORT param it defaults to 4000 - I think this is just a doc issue of "how to I make that clearer"