Merge pull request #130 from kubeflow/main

[pull] main from kubeflow:main

.github/workflows/ui-bff-build.yml

@@ -24,6 +24,12 @@ jobs:
         working-directory: clients/ui/bff
         run: make clean
 
+      - name: Lint
+        uses: golangci/golangci-lint-action@v6
+        with:
+          version: v1.57.2
+          working-directory: clients/ui/bff/
+
       - name: Build
         working-directory: clients/ui/bff
         run: make build

clients/ui/bff/Makefile

@@ -19,6 +19,14 @@ fmt:
 clean:
 	rm -Rf ./bin
 
+.PHONY: lint
+lint: golangci-lint ## Run golangci-lint linter
+	$(GOLANGCI_LINT) run
+
+.PHONY: lint-fix
+lint-fix: golangci-lint ## Run golangci-lint linter and perform fixes
+	$(GOLANGCI_LINT) run --fix
+
 .PHONY: vet
 vet:  .
 	go vet ./...
@@ -38,3 +46,33 @@ run: fmt vet
 .PHONY: docker-build
 docker-build:
 	$(CONTAINER_TOOL) build -t ${IMG} .
+
+##@ Dependencies
+
+## Location to install dependencies to
+LOCALBIN ?= $(shell pwd)/bin
+$(LOCALBIN):
+	mkdir -p $(LOCALBIN)
+
+GOLANGCI_LINT ?= $(LOCALBIN)/golangci-lint-$(GOLANGCI_LINT_VERSION)
+GOLANGCI_LINT_VERSION ?= v1.57.2
+
+.PHONY: golangci-lint
+golangci-lint: $(GOLANGCI_LINT) ## Download golangci-lint locally if necessary.
+$(GOLANGCI_LINT): $(LOCALBIN)
+	$(call go-install-tool,$(GOLANGCI_LINT),github.com/golangci/golangci-lint/cmd/golangci-lint,${GOLANGCI_LINT_VERSION})
+
+
+# go-install-tool will 'go install' any package with custom target and name of binary, if it doesn't exist
+# $1 - target path with name of binary (ideally with version)
+# $2 - package url which can be installed
+# $3 - specific version of package
+define go-install-tool
+@[ -f $(1) ] || { \
+set -e; \
+package=$(2)@$(3) ;\
+echo "Downloading $${package}" ;\
+GOBIN=$(LOCALBIN) go install $${package} ;\
+mv "$$(echo "$(1)" | sed "s/-$(3)$$//")" $(1) ;\
+}
+endef

clients/ui/bff/README.md

@@ -188,4 +188,24 @@ curl -i -X POST "http://localhost:4000/api/v1/model_registry/model-registry/mode
   "state": "LIVE",
   "artifactType": "TYPE_ONE"
 }}'
-```
+```
+
+### Pagination
+The following query parameters are supported by "Get All" style endpoints to control pagination.
+
+| Parameter Name | Description                                                                                               |
+|----------------|-----------------------------------------------------------------------------------------------------------|
+| pageSize       | Number of entities in each page                                                                           |
+| orderBy        | Specifies the order by criteria for listing entities. Available values: CREATE_TIME, LAST_UPDATE_TIME, ID |
+| sortOrder      | Specifies the sort order for listing entities. Available values: ASC, DESC. Default: ASC                  |
+| nextPageToken  | Token to use to retrieve next page of results.                                                            | 
+
+### Sample local calls
+```
+# Get with a page size of 5 getting a specific page.
+curl -i "http://localhost:4000/api/v1/model_registry/model-registry/registered_models?pageSize=5&nextPageToken=CAEQARoCCAE"
+```
+```
+# Get with a page size of 5, order by last update time in descending order.
+curl -i "http://localhost:4000/api/v1/model_registry/model-registry/registered_models?pageSize=5&orderBy=LAST_UPDATE_TIME&sortOrder=DESC"
+```

clients/ui/bff/docs/contributing.md

@@ -0,0 +1,11 @@
+# Contributing
+
+## Running the linter locally
+The BFF directory uses golangci-lint to combine multiple linters for a more comprehensive linting process. To install and run simply use:
+
+```shell
+cd clients/ui/bff
+make lint
+```
+
+For more information on configuring golangci-lint see the [documentation](https://golangci-lint.run/).

clients/ui/bff/go.mod

@@ -5,7 +5,7 @@ go 1.22.2
 require (
 	github.com/brianvoe/gofakeit/v7 v7.0.4
 	github.com/julienschmidt/httprouter v1.3.0
-	github.com/kubeflow/model-registry v0.2.6-alpha
+	github.com/kubeflow/model-registry v0.2.7-alpha
 	github.com/stretchr/testify v1.9.0
 	k8s.io/api v0.31.1
 	k8s.io/apimachinery v0.31.1

clients/ui/bff/go.sum

@@ -53,8 +53,8 @@ github.com/kr/pty v1.1.1/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ=
 github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI=
 github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY=
 github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE=
-github.com/kubeflow/model-registry v0.2.6-alpha h1:5+HXFeiHTRxYJhfCuovO/icD6xtcTFxHJRzMkDgIyfw=
-github.com/kubeflow/model-registry v0.2.6-alpha/go.mod h1:exMa9sONJSHHLvK8kcPfh+v1+7jvTY9z8e3TmaKjZXU=
+github.com/kubeflow/model-registry v0.2.7-alpha h1:5F5eb6ZsXsC6g1NAkYB9l0AYqlKZo+NOKlzJqFy4ROI=
+github.com/kubeflow/model-registry v0.2.7-alpha/go.mod h1:exMa9sONJSHHLvK8kcPfh+v1+7jvTY9z8e3TmaKjZXU=
 github.com/mailru/easyjson v0.7.7 h1:UGYAvKxe3sBsEDzO8ZeWOSlIQfWFlxbzLZe7hwFURr0=
 github.com/mailru/easyjson v0.7.7/go.mod h1:xzfreul335JAWq5oZzymOObrkdz5UnU4kGfJJLY9Nlc=
 github.com/modern-go/concurrent v0.0.0-20180228061459-e0a39a4cb421/go.mod h1:6dJC0mAP4ikYIbvyc7fijjWJddQyLn8Ig3JB5CqoB9Q=

clients/ui/bff/internal/api/errors.go

@@ -3,9 +3,10 @@ package api
 import (
 	"encoding/json"
 	"fmt"
-	"github.com/kubeflow/model-registry/ui/bff/internal/integrations"
 	"net/http"
 	"strconv"
+
+	"github.com/kubeflow/model-registry/ui/bff/internal/integrations"
 )
 
 type HTTPError struct {
@@ -91,7 +92,8 @@ func (app *App) methodNotAllowedResponse(w http.ResponseWriter, r *http.Request)
 	app.errorResponse(w, r, httpError)
 }
 
-func (app *App) failedValidationResponse(w http.ResponseWriter, r *http.Request, errors map[string]string) {
+// TODO remove nolint comment below when we use this method
+func (app *App) failedValidationResponse(w http.ResponseWriter, r *http.Request, errors map[string]string) { //nolint:unused
 
 	message, err := json.Marshal(errors)
 	if err != nil {

clients/ui/bff/internal/api/model_registry_handler.go

@@ -8,7 +8,7 @@ import (
 
 type ModelRegistryListEnvelope Envelope[[]data.ModelRegistryModel, None]
 
-func (app *App) ModelRegistryHandler(w http.ResponseWriter, r *http.Request, ps httprouter.Params) {
+func (app *App) ModelRegistryHandler(w http.ResponseWriter, r *http.Request, _ httprouter.Params) {
 
 	registries, err := app.models.ModelRegistry.FetchAllModelRegistries(app.kubernetesClient)
 	if err != nil {

clients/ui/bff/internal/api/model_versions_handler.go

@@ -46,7 +46,7 @@ func (app *App) GetModelVersionHandler(w http.ResponseWriter, r *http.Request, p
 	}
 }
 
-func (app *App) CreateModelVersionHandler(w http.ResponseWriter, r *http.Request, ps httprouter.Params) {
+func (app *App) CreateModelVersionHandler(w http.ResponseWriter, r *http.Request, _ httprouter.Params) {
 	client, ok := r.Context().Value(httpClientKey).(integrations.HTTPClientInterface)
 	if !ok {
 		app.serverErrorResponse(w, r, errors.New("REST client not found"))
@@ -157,7 +157,7 @@ func (app *App) GetAllModelArtifactsByModelVersionHandler(w http.ResponseWriter,
 		return
 	}
 
-	data, err := app.modelRegistryClient.GetModelArtifactsByModelVersion(client, ps.ByName(ModelVersionId))
+	data, err := app.modelRegistryClient.GetModelArtifactsByModelVersion(client, ps.ByName(ModelVersionId), r.URL.Query())
 	if err != nil {
 		app.serverErrorResponse(w, r, err)
 		return

clients/ui/bff/internal/api/registered_models_handler.go

@@ -4,27 +4,25 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"github.com/julienschmidt/httprouter"
+	"github.com/kubeflow/model-registry/pkg/openapi"
 	"github.com/kubeflow/model-registry/ui/bff/internal/integrations"
 	"github.com/kubeflow/model-registry/ui/bff/internal/validation"
 	"net/http"
-
-	"github.com/julienschmidt/httprouter"
-	"github.com/kubeflow/model-registry/pkg/openapi"
 )
 
 type RegisteredModelEnvelope Envelope[*openapi.RegisteredModel, None]
 type RegisteredModelListEnvelope Envelope[*openapi.RegisteredModelList, None]
 type RegisteredModelUpdateEnvelope Envelope[*openapi.RegisteredModelUpdate, None]
 
-func (app *App) GetAllRegisteredModelsHandler(w http.ResponseWriter, r *http.Request, ps httprouter.Params) {
-	//TODO (ederign) implement pagination
+func (app *App) GetAllRegisteredModelsHandler(w http.ResponseWriter, r *http.Request, _ httprouter.Params) {
 	client, ok := r.Context().Value(httpClientKey).(integrations.HTTPClientInterface)
 	if !ok {
 		app.serverErrorResponse(w, r, errors.New("REST client not found"))
 		return
 	}
 
-	modelList, err := app.modelRegistryClient.GetAllRegisteredModels(client)
+	modelList, err := app.modelRegistryClient.GetAllRegisteredModels(client, r.URL.Query())
 	if err != nil {
 		app.serverErrorResponse(w, r, err)
 		return
@@ -40,7 +38,7 @@ func (app *App) GetAllRegisteredModelsHandler(w http.ResponseWriter, r *http.Req
 	}
 }
 
-func (app *App) CreateRegisteredModelHandler(w http.ResponseWriter, r *http.Request, ps httprouter.Params) {
+func (app *App) CreateRegisteredModelHandler(w http.ResponseWriter, r *http.Request, _ httprouter.Params) {
 	client, ok := r.Context().Value(httpClientKey).(integrations.HTTPClientInterface)
 	if !ok {
 		app.serverErrorResponse(w, r, errors.New("REST client not found"))
@@ -173,14 +171,13 @@ func (app *App) UpdateRegisteredModelHandler(w http.ResponseWriter, r *http.Requ
 }
 
 func (app *App) GetAllModelVersionsForRegisteredModelHandler(w http.ResponseWriter, r *http.Request, ps httprouter.Params) {
-	//TODO (acreasy) implement pagination
 	client, ok := r.Context().Value(httpClientKey).(integrations.HTTPClientInterface)
 	if !ok {
 		app.serverErrorResponse(w, r, errors.New("REST client not found"))
 		return
 	}
 
-	versionList, err := app.modelRegistryClient.GetAllModelVersions(client, ps.ByName(RegisteredModelId))
+	versionList, err := app.modelRegistryClient.GetAllModelVersions(client, ps.ByName(RegisteredModelId), r.URL.Query())
 
 	if err != nil {
 		app.serverErrorResponse(w, r, err)

clients/ui/bff/internal/data/helpers.go

@@ -0,0 +1,38 @@
+package data
+
+import (
+	"fmt"
+	"net/url"
+)
+
+func FilterPageValues(values url.Values) url.Values {
+	result := url.Values{}
+
+	if v := values.Get("pageSize"); v != "" {
+		result.Set("pageSize", v)
+	}
+	if v := values.Get("orderBy"); v != "" {
+		result.Set("orderBy", v)
+	}
+	if v := values.Get("sortOrder"); v != "" {
+		result.Set("sortOrder", v)
+	}
+	if v := values.Get("nextPageToken"); v != "" {
+		result.Set("nextPageToken", v)
+	}
+
+	return result
+}
+
+func UrlWithParams(url string, values url.Values) string {
+	queryString := values.Encode()
+	if queryString == "" {
+		return url
+	}
+	return fmt.Sprintf("%s?%s", url, queryString)
+}
+
+func UrlWithPageParams(url string, values url.Values) string {
+	pageValues := FilterPageValues(values)
+	return UrlWithParams(url, pageValues)
+}

clients/ui/bff/internal/data/model_version.go

@@ -16,7 +16,7 @@ type ModelVersionInterface interface {
 	GetModelVersion(client integrations.HTTPClientInterface, id string) (*openapi.ModelVersion, error)
 	CreateModelVersion(client integrations.HTTPClientInterface, jsonData []byte) (*openapi.ModelVersion, error)
 	UpdateModelVersion(client integrations.HTTPClientInterface, id string, jsonData []byte) (*openapi.ModelVersion, error)
-	GetModelArtifactsByModelVersion(client integrations.HTTPClientInterface, id string) (*openapi.ModelArtifactList, error)
+	GetModelArtifactsByModelVersion(client integrations.HTTPClientInterface, id string, pageValues url.Values) (*openapi.ModelArtifactList, error)
 	CreateModelArtifactByModelVersion(client integrations.HTTPClientInterface, id string, jsonData []byte) (*openapi.ModelArtifact, error)
 }
 
@@ -79,14 +79,14 @@ func (v ModelVersion) UpdateModelVersion(client integrations.HTTPClientInterface
 	return &model, nil
 }
 
-func (v ModelVersion) GetModelArtifactsByModelVersion(client integrations.HTTPClientInterface, id string) (*openapi.ModelArtifactList, error) {
+func (v ModelVersion) GetModelArtifactsByModelVersion(client integrations.HTTPClientInterface, id string, pageValues url.Values) (*openapi.ModelArtifactList, error) {
 	path, err := url.JoinPath(modelVersionPath, id, artifactsByModelVersionPath)
 
 	if err != nil {
 		return nil, err
 	}
 
-	responseData, err := client.GET(path)
+	responseData, err := client.GET(UrlWithPageParams(path, pageValues))
 	if err != nil {
 		return nil, fmt.Errorf("error fetching model version artifacts: %w", err)
 	}

clients/ui/bff/internal/data/model_version_test.go

@@ -2,17 +2,19 @@ package data
 
 import (
 	"encoding/json"
+	"fmt"
+	"net/http"
+	"net/url"
+	"testing"
+
 	"github.com/brianvoe/gofakeit/v7"
 	"github.com/kubeflow/model-registry/ui/bff/internal/mocks"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
-	"net/http"
-	"net/url"
-	"testing"
 )
 
 func TestGetModelVersion(t *testing.T) {
-	gofakeit.Seed(0)
+	_ = gofakeit.Seed(0)
 
 	expected := mocks.GenerateMockModelVersion()
 
@@ -37,7 +39,7 @@ func TestGetModelVersion(t *testing.T) {
 }
 
 func TestCreateModelVersion(t *testing.T) {
-	gofakeit.Seed(0)
+	_ = gofakeit.Seed(0)
 
 	expected := mocks.GenerateMockModelVersion()
 
@@ -62,7 +64,7 @@ func TestCreateModelVersion(t *testing.T) {
 }
 
 func TestUpdateModelVersion(t *testing.T) {
-	gofakeit.Seed(0)
+	_ = gofakeit.Seed(0)
 
 	expected := mocks.GenerateMockModelVersion()
 
@@ -90,7 +92,7 @@ func TestUpdateModelVersion(t *testing.T) {
 }
 
 func TestGetModelArtifactsByModelVersion(t *testing.T) {
-	gofakeit.Seed(0)
+	_ = gofakeit.Seed(0)
 
 	expected := mocks.GenerateMockModelArtifactList()
 
@@ -105,7 +107,7 @@ func TestGetModelArtifactsByModelVersion(t *testing.T) {
 	mockClient := new(mocks.MockHTTPClient)
 	mockClient.On(http.MethodGet, path, mock.Anything).Return(mockData, nil)
 
-	actual, err := modelVersion.GetModelArtifactsByModelVersion(mockClient, "1")
+	actual, err := modelVersion.GetModelArtifactsByModelVersion(mockClient, "1", nil)
 	assert.NoError(t, err)
 
 	assert.NotNil(t, actual)
@@ -115,8 +117,33 @@ func TestGetModelArtifactsByModelVersion(t *testing.T) {
 	assert.Equal(t, len(expected.Items), len(actual.Items))
 }
 
+func TestGetModelArtifactsByModelVersionWithPageParams(t *testing.T) {
+	gofakeit.Seed(0) //nolint:errcheck
+
+	pageValues := mocks.GenerateMockPageValues()
+	expected := mocks.GenerateMockModelArtifactList()
+
+	mockData, err := json.Marshal(expected)
+	assert.NoError(t, err)
+
+	modelVersion := ModelVersion{}
+
+	path, err := url.JoinPath(modelVersionPath, "1", artifactsByModelVersionPath)
+	assert.NoError(t, err)
+	reqUrl := fmt.Sprintf("%s?%s", path, pageValues.Encode())
+
+	mockClient := new(mocks.MockHTTPClient)
+	mockClient.On(http.MethodGet, reqUrl, mock.Anything).Return(mockData, nil)
+
+	actual, err := modelVersion.GetModelArtifactsByModelVersion(mockClient, "1", pageValues)
+	assert.NoError(t, err)
+
+	assert.NotNil(t, actual)
+	mockClient.AssertExpectations(t)
+}
+
 func TestCreateModelArtifactByModelVersion(t *testing.T) {
-	gofakeit.Seed(0)
+	_ = gofakeit.Seed(0)
 
 	expected := mocks.GenerateMockModelArtifact()