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

Create BuildListOption for REST query params #139

Merged
merged 3 commits into from
Nov 14, 2023

Conversation

tarilabs
Copy link
Member

Resolves #138
as discussed with @lampajr

Description

Ignores unsupplied-but-golang-empty query params, and relies on defaults by gRPC; the defaults could now be overridden for alternative default-values in a single place if needed later.

How Has This Been Tested?

After Build and a MLMD available on docker:

robot test/robot/UserStory.robot
curl -X 'GET' 'http://localhost:8080/api/model_registry/v1alpha1/registered_models?pageSize=47' -H 'accept: application/json'
curl -X 'GET' 'http://localhost:8080/api/model_registry/v1alpha1/registered_models' -H 'accept: application/json'

Merge criteria:

  • The commits and have meaningful messages; the author will squash them after approval or 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

@tarilabs tarilabs requested a review from a team November 10, 2023 11:34
lampajr
lampajr previously approved these changes Nov 10, 2023
Copy link
Contributor

@lampajr lampajr left a comment

Choose a reason for hiding this comment

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

lgmt, left just one minor comment

internal/core/api_utils.go Outdated Show resolved Hide resolved
lampajr
lampajr previously approved these changes Nov 10, 2023
Copy link
Contributor

@lampajr lampajr left a comment

Choose a reason for hiding this comment

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

Rebased from main and added commit a55f7be to fix the image container build issue.

Note that the check here is still failing because for security reason we are using pull_request_target event, this way changes applied on this PR are not actually tested in the container build --> raised issue opendatahub-io/model-registry#145 to keep track of this.

The change lgtm!

@lampajr lampajr merged commit 9d9d48f into opendatahub-io:main Nov 14, 2023
1 of 2 checks passed
@lampajr lampajr mentioned this pull request Nov 14, 2023
3 tasks
dhirajsb pushed a commit to dhirajsb/model-registry that referenced this pull request Jan 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing query params for pagination should be ignored
2 participants