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

Feature: Index Schema Field Filtering #202

Merged
merged 56 commits into from
Apr 8, 2024

Conversation

michael-valdron
Copy link
Member

@michael-valdron michael-valdron commented Mar 5, 2024

Please specify the area for this PR

What does does this PR do / why we need it:

Adds field-based filtering of index schemas with fuzzy search and and logical based filter stacking, e.g. filter by name and description.

Additional Changes:

  • Added deprecated filter parameter and filter process: REST API query parameters for requesting field-based filtering of index schemas api#959 (comment)
  • Endpoint unit tests now use generated ServerInterfaceWrapper rather than explicitly defined Server that implements generated ServerInterface to better test parameter wrapper along with greatly increasing code coverage, see endpoint_test.go#L481 for an example of this change
  • Use of github.com/mohae/deepcopy in filter to produce a different copy of the filtered index schema
  • Use of github.com/hashicorp/go-set to use sets where it is more efficient
  • Use of k8s.io/apiextensions-apiserver to set field pointers easier
  • Introduction of new helper functions
    • StructToMap to transform a struct type into a map[string]any
    • StrPtrIsSet to return the result of ptr != nil && *ptr != "" for string pointers
  • Introduction of IndexParams and toIndexParams receiver functions to generalize index endpoint parameter types from generated source
  • Removal of GET method parameters from not allowed method handlers
  • Correct typos under endpoint unit tests

Which issue(s) this PR fixes:

Fixes #?

fixes devfile/api#959

PR acceptance criteria:

  • Test Coverage
    • Are your changes sufficiently tested, and are any applicable test cases added or updated to cover your changes?

Documentation (WIP)

How to test changes / Special notes to the reviewer:

  1. Build devfile-index image from change
  2. Deploy built devfile-index image
  3. Build & run altered integration test suite on deployment of built devfile-index from step 1
  4. Run cd index/server && go test ./... to run unit tests on changes

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 5, 2024
Copy link

codecov bot commented Mar 5, 2024

Codecov Report

Attention: Patch coverage is 54.76540% with 617 lines in your changes are missing coverage. Please review.

Project coverage is 41.16%. Comparing base (7c89891) to head (5b6f1cf).
Report is 1 commits behind head on main.

Files Patch % Lines
index/server/pkg/server/endpoint.gen.go 23.53% 380 Missing and 91 partials ⚠️
index/server/pkg/util/filter.go 76.62% 110 Missing and 5 partials ⚠️
index/server/pkg/server/endpoint.go 75.00% 15 Missing and 4 partials ⚠️
index/server/pkg/util/util.go 77.14% 5 Missing and 3 partials ⚠️
index/server/pkg/server/filter.go 86.20% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #202       +/-   ##
===========================================
+ Coverage   27.27%   41.16%   +13.88%     
===========================================
  Files           7        9        +2     
  Lines        2009     3102     +1093     
===========================================
+ Hits          548     1277      +729     
- Misses       1414     1636      +222     
- Partials       47      189      +142     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@michael-valdron michael-valdron force-pushed the feat/index-filtering branch 7 times, most recently from cccdb89 to 50d7bac Compare March 19, 2024 15:13
@michael-valdron michael-valdron force-pushed the feat/index-filtering branch 3 times, most recently from 715dfec to 6ba44d4 Compare March 25, 2024 22:11
@michael-valdron michael-valdron changed the title [WIP] Feature: Index Schema Field Filtering Feature: Index Schema Field Filtering Mar 27, 2024
@michael-valdron michael-valdron requested review from thepetk, Jdubrick and a team March 27, 2024 20:05
@michael-valdron michael-valdron marked this pull request as ready for review March 27, 2024 20:05
@openshift-ci openshift-ci bot requested a review from elsony March 27, 2024 20:05
Copy link
Contributor

@thepetk thepetk left a comment

Choose a reason for hiding this comment

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

@michael-valdron I've tried to build locally and test the new devfile-index image with minikube. When I'm trying to get filtered results I get the full list of stacks. What I'm trying to fetch is:

http://devfile-registry-default.<IP>.nip.io/v2index?name=python

Any ideas on what I'm doing wrong?

The dockerfile I'm using to build the image is:

FROM registry.access.redhat.com/ubi8/go-toolset:1.19 AS builder

# Set user as root
USER root

# Install yq
RUN curl -sL -O https://github.com/mikefarah/yq/releases/download/v4.9.5/yq_linux_amd64 -o /usr/local/bin/yq && mv ./yq_linux_amd64 /usr/local/bin/yq && chmod +x /usr/local/bin/yq

COPY . /registry

# Download the registry build tools
RUN git clone --branch feat/index-filtering https://github.com/michael-valdron/devfile-registry-support.git /registry-support

# Run the registry build tools
RUN bash /registry-support/build-tools/build.sh /registry /build

# Set user as non-root
USER 1001

FROM quay.io/devfile/devfile-index-base:next

COPY --from=builder /build /registry

@michael-valdron
Copy link
Member Author

michael-valdron commented Apr 3, 2024

@michael-valdron I've tried to build locally and test the new devfile-index image with minikube. When I'm trying to get filtered results I get the full list of stacks. What I'm trying to fetch is:

http://devfile-registry-default.<IP>.nip.io/v2index?name=python

@thepetk I wasn't able to reproduce exactly what you got (all stacks), I got an empty result.

The empty result would be because this endpoint /v2index (and /v2index/stack) only queries stacks whereas the only python entry in the test registry is a sample, you can pull the desired result with:

curl http://devfile-registry-default.<IP>.nip.io/v2index/sample?name=python

You can filter this against stacks and samples with:

curl http://devfile-registry-default.<IP>.nip.io/v2index/all?name=python

@michael-valdron
Copy link
Member Author

When I'm trying to get filtered results I get the full list of stacks.

I wasn't able to reproduce exactly what you got (all stacks)

@thepetk What steps did you do to build and deploy? Maybe there is something else missed.

@thepetk
Copy link
Contributor

thepetk commented Apr 4, 2024

curl http://devfile-registry-default.<IP>.nip.io/v2index/sample?name=python

I'll re-do again all steps just to avoid the case that the devfile-index image wasn't up-to-date

Copy link
Contributor

@thepetk thepetk left a comment

Choose a reason for hiding this comment

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

@michael-valdron as we've checked together (thanks for the help debugging my issue!), I'd suggest adding a custom field for deprecated stacks.

This way when someone needs to fetch a deprecated stack (a stack with the Deprecated tag inside the default tags) will fetch only the fully deprecated stacks (without a supported version).

e.g

https://<host>/v2index/all?deprecated=true

@thepetk
Copy link
Contributor

thepetk commented Apr 4, 2024

@michael-valdron I've tried to build locally and test the new devfile-index image with minikube. When I'm trying to get filtered results I get the full list of stacks. What I'm trying to fetch is:

http://devfile-registry-default.<IP>.nip.io/v2index?name=python

Any ideas on what I'm doing wrong?

The dockerfile I'm using to build the image is:

FROM registry.access.redhat.com/ubi8/go-toolset:1.19 AS builder

# Set user as root
USER root

# Install yq
RUN curl -sL -O https://github.com/mikefarah/yq/releases/download/v4.9.5/yq_linux_amd64 -o /usr/local/bin/yq && mv ./yq_linux_amd64 /usr/local/bin/yq && chmod +x /usr/local/bin/yq

COPY . /registry

# Download the registry build tools
RUN git clone --branch feat/index-filtering https://github.com/michael-valdron/devfile-registry-support.git /registry-support

# Run the registry build tools
RUN bash /registry-support/build-tools/build.sh /registry /build

# Set user as non-root
USER 1001

FROM quay.io/devfile/devfile-index-base:next

COPY --from=builder /build /registry

For everyone's information the issue here was the quay.io/devfile/devfile-index-base:next. In order to be able to properly test the changes I had to build the base image from the base branch.

@Jdubrick
Copy link
Contributor

Jdubrick commented Apr 4, 2024

Can't seem to build and run the integration tests because my podman alias is not being properly set so I will need to look into this before I can run the tests on my end. I was able to build and deploy the devfile-index however and seemed to work with no issues.

Screenshot 2024-04-04 at 4 12 36 PM

@michael-valdron
Copy link
Member Author

Can't seem to build and run the integration tests because my podman alias is not being properly set so I will need to look into this before I can run the tests on my end. I was able to build and deploy the devfile-index however and seemed to work with no issues.

Screenshot 2024-04-04 at 4 12 36 PM

Opened devfile/api#1502 to address this along with devfile/api#1503 to discover any other missing/broken podman support in our registry-support scripts.

FYI @Jdubrick @thepetk

@michael-valdron michael-valdron requested a review from thepetk April 5, 2024 18:24
@michael-valdron
Copy link
Member Author

@michael-valdron as we've checked together (thanks for the help debugging my issue!), I'd suggest adding a custom field for deprecated stacks.

This way when someone needs to fetch a deprecated stack (a stack with the Deprecated tag inside the default tags) will fetch only the fully deprecated stacks (without a supported version).

e.g

https://<host>/v2index/all?deprecated=true

I have included this additional parameter with the some criteria: devfile/api#959 (comment)

FYI @thepetk @Jdubrick

Copy link
Contributor

@thepetk thepetk left a comment

Choose a reason for hiding this comment

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

/lgtm tested locally all the filters & the additional deprecated=true/false :)

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 8, 2024
Copy link

openshift-ci bot commented Apr 8, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: michael-valdron, thepetk

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:
  • OWNERS [michael-valdron,thepetk]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@michael-valdron michael-valdron merged commit b950a06 into devfile:main Apr 8, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

REST API query parameters for requesting field-based filtering of index schemas
4 participants