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

Add filtering by annotations on search #114

Merged
merged 1 commit into from
Apr 23, 2021

Conversation

Itxaka
Copy link
Contributor

@Itxaka Itxaka commented Apr 7, 2021

Allows to add filtering by annotations by passing the -a flag and the
annotation in the format "KEY=VALUE"

So you can search like this:

hypper search repo -a "catalog.cattle.io/certified=rancher"

And it will only show charts that have that exact annotation.

Requires #113

Signed-off-by: Itxaka igarcia@suse.com

@Itxaka Itxaka force-pushed the filterannotation branch 3 times, most recently from 6b81217 to 3f63272 Compare April 8, 2021 10:30
@Itxaka Itxaka linked an issue Apr 9, 2021 that may be closed by this pull request
@Itxaka Itxaka added enhancement New feature or request ready for review Somebody please take a look at this and review labels Apr 13, 2021
name: "search for hypper, expect latest version 0.2.0",
cmd: "search repo hypper --output yaml",
golden: "output/search-hypper-latest.txt",
}, { // I use yaml for the output because its easier to create the golden files, sue me :p
Copy link
Member

Choose a reason for hiding this comment

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

🤣 🤣 🤣 top!

@viccuad viccuad self-requested a review April 13, 2021 13:08
viccuad
viccuad previously approved these changes Apr 13, 2021
Copy link
Member

@viccuad viccuad left a comment

Choose a reason for hiding this comment

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

nice! looks nice and clean, the docs are slowly growing :D.

@Itxaka
Copy link
Contributor Author

Itxaka commented Apr 13, 2021

Rebased!

pkg/search/search.go Outdated Show resolved Hide resolved
@Itxaka Itxaka requested a review from viccuad April 14, 2021 07:57
pkg/search/search.go Outdated Show resolved Hide resolved
pkg/search/index.go Show resolved Hide resolved
Allows to add filtering by annotations by passing the -a flag and the
annotation in the format "KEY=VALUE"

So you can search like this:
hypper search repo -a "catalog.cattle.io/certified=rancher"
And it will only show charts that have that exact annotation.

Signed-off-by: Itxaka <igarcia@suse.com>
Copy link
Collaborator

@mattfarina mattfarina left a comment

Choose a reason for hiding this comment

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

I was looking over #93 (the issue with the requirements for this). The idea is that Rancher has 1 charts repo and multiple versions of Rancher use it. Some chart versions will not work with some versions of Rancher. So, a version range of supported rancher versions on the chart could be used to filter for known supported versions.

To make this work we need to have more than a check on the existence of an annotation key/value pair.

For that same issue we need to expose this via a Go API. That can be a separate PR, though.

@Itxaka
Copy link
Contributor Author

Itxaka commented Apr 15, 2021

I dont understand @mattfarina

This covers your user case correctly. You can use this to search for -a "catalog.cattle.io/rancher-version=2.5.*" --regex for example and it would search for that with regex in versioning.

So this perfectly covers that.

Now, please, if you have a specific case in mind, post it in the issue because if the issue is not clear or not defined enough, we are wasting time writing code that has to be refactored.

This covers much more ground that the specific issue and its flexible enough to be used perfectly on hypper, plus brings value into hypper by making it all about annotations.

@viccuad
Copy link
Member

viccuad commented Apr 16, 2021

What I understood from Matt is that, in addition to filtering on annotations, we want to have a semver ranges aware filter. So we could ask for things matching ~1.2.3, or ^1.2.3, etc.
Edit: Helm seems to be using "github.com/Masterminds/semver/v3": https://github.com/helm/helm/blob/167aac70832d3a384f65f9745335e9fb40169dc2/cmd/helm/search_repo.go#L145

@Itxaka
Copy link
Contributor Author

Itxaka commented Apr 16, 2021

What I understood from Matt is that, in addition to filtering on annotations, we want to have a semver ranges aware filter. So we could ask for things matching ~1.2.3, or ^1.2.3, etc.

but then we are not being consequent here.

Either we want to use a exact key to search in the annotations and make that semver aware, or we want this to not be rancher agnostic.

Like if we are gonna search for catalog.cattle.io/rancher-version we are tying up ourselves to rancher, but its clear on the ticket that this is not desired....so...Im not sure.

If there is a key we want to search for that we know contains semver then we should say that in the ticket, otherwise we get a generic, not-tied-to-rancher, annotation filter search.

Sure, we can be reckless and just try to load the values from the annotations into a semver constraint and fail otherwise but that is such a brute force approach that it makes no sense to me.

Again, the ticket doesnt seem to be clear. Annotations can be of any type. If we want to search for just one annotation that contains the rancher version and do a semver constraint on there, good for me, I just need to know which annotation so we can discard the rest. Otherwise, lets build a more intelligent, generic annotation search that can be used to search also for hypper annotations!

We seem to have an identity issue in which we either want hypper to be this community tool with a clear goal and suddenly we move as making hypper a vessel for rancher use cases. And this makes the hypper vision not clear enough IMO.

In any case, I still think this bring value to hypper as it allows use to center ourselves on annotations and search for those.

Copy link
Member

@viccuad viccuad left a comment

Choose a reason for hiding this comment

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

Approving, the PR is nice and has tests! If we need to change or add filters, we can do it in subsequent PRs.

Copy link
Collaborator

@mattfarina mattfarina left a comment

Choose a reason for hiding this comment

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

Let's hold off on this until after the v0.1.0 release.

In our meeting with Darren he had wanted semver range resolution as a feature. It was more than exact key/value pair checking. I will message him to double check what they are looking for.

@mattfarina mattfarina merged commit 5f063bc into rancher-sandbox:main Apr 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ready for review Somebody please take a look at this and review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Filter based on annotations
4 participants