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 search command #113

Merged
merged 1 commit into from
Apr 13, 2021
Merged

Add search command #113

merged 1 commit into from
Apr 13, 2021

Conversation

Itxaka
Copy link
Contributor

@Itxaka Itxaka commented Apr 7, 2021

Adds the helm repo search command to hypper as just hypper search

Under helm you can search on the repos or on their own hub. This
patch only implements the repo search as seems the most useful
and bringing the hub requires a lot of code

This patch also extracts the search code into a new pkg as under
helm the search code is mostly crammed under cmd which means you cannot
use it anywhere, meanwhile this code can be reused by third party
clients to implement their own search frontend or automate it.

Fixes: #78

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

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! having the search functionality in the libs sounds awesome :)

I have only done a fast review so far. Could we first double-check on the exported functions and methods that will be in the lib API? For example, maybe we don't need to expose WriteJSON() and WriteYAML(). Also, could we add comments to the exported ones? (it would help me with the review).

@Itxaka
Copy link
Contributor Author

Itxaka commented Apr 7, 2021

For example, maybe we don't need to expose WriteJSON() and WriteYAML().

Unfortunately due to the convoluted way in which those functions are called they need to be exposed because repoSearchWriter needs to implement the Writer interface in order to be called.

Also, could we add comments to the exported ones?

Will do!

pkg/search/search.go Outdated Show resolved Hide resolved
@Itxaka Itxaka force-pushed the search branch 2 times, most recently from 3cfacc5 to 2c9e381 Compare April 8, 2021 09:55
@Itxaka Itxaka added the ready for review Somebody please take a look at this and review label Apr 8, 2021
@Itxaka
Copy link
Contributor Author

Itxaka commented Apr 8, 2021

user docs added!

pkg/search/search.go Outdated Show resolved Hide resolved
@Itxaka Itxaka requested a review from viccuad April 8, 2021 11:24
cmd/hypper/search.go Outdated Show resolved Hide resolved
@Itxaka Itxaka requested a review from viccuad April 9, 2021 09:37
viccuad
viccuad previously approved these changes Apr 9, 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.

Approving, as as we talked on the chat, is gonna get rebased.

docs/user/howto/search.md Outdated Show resolved Hide resolved
docs/user/howto/search.md Outdated Show resolved Hide resolved
pkg/search/search.go Outdated Show resolved Hide resolved
@viccuad viccuad self-requested a review April 13, 2021 12:57
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.

Wooohoo! nice, and with the cmd/helm/search things in our own pkg/search!

Approving, feel free the consider my reviews as passed if/when typos get pushed!

viccuad
viccuad previously approved these changes Apr 13, 2021
Adds the helm repo search command to hypper as just hypper search

Under helm you can search on the repos or on their own hub. This
patch only implements the repo search as seems the most useful
and bringing the hub requires a lot of code. There is still room to
bring the hub search into hypper if deemed necessary

This patch also extracts the search code into a new pkg as under
helm the search code is mostly crammed under cmd which means you cannot
use it anywhere, meanwhile this code can be reused by third party
clients to implement their own search frontend or automate it.

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

Itxaka commented Apr 13, 2021

rebasing for the conflicts...

@Itxaka Itxaka requested a review from viccuad April 13, 2021 14:19
@Itxaka
Copy link
Contributor Author

Itxaka commented Apr 13, 2021

was already approved by victor, I had to rebase due to merge conflicts on the root.go file (because lint added a command there and we are also adding a command)

So merging :)

@Itxaka Itxaka merged commit 5f341e7 into rancher-sandbox:main Apr 13, 2021
@Itxaka Itxaka deleted the search branch April 13, 2021 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

hypper search
2 participants