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.

Sign up for GitHub

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

Jump to bottom

check all existing index #751

Merged
merged 3 commits into from
Jan 14, 2022
Merged

check all existing index #751

merged 3 commits into from
Jan 14, 2022

Conversation

junnplus
Copy link
Contributor

@junnplus junnplus commented Dec 30, 2021 •
edited by ahmetb
Loading

Fixes #750.

We shouldn't just check the default index.

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Dec 30, 2021 •
edited
Loading

CLA Signed

The committers are authorized under a signed CLA.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Dec 30, 2021
@k8s-ci-robot
Copy link
Contributor

Welcome @junnplus!

It looks like this is your first PR to kubernetes-sigs/krew 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/krew has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 30, 2021
@junnplus junnplus force-pushed the check-index branch 3 times, most recently from 7d34838 to cbb6b47 Compare December 30, 2021 03:47
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Dec 30, 2021
@junnplus junnplus mentioned this pull request Dec 30, 2021
Closed
chriskim06
chriskim06 reviewed Dec 30, 2021
View reviewed changes
cmd/krew/cmd/root.go Outdated
if ok, err := gitutil.IsGitCloned(indexPath); err != nil {
return errors.Wrap(err, "failed to check local index git repository")
} else if !ok {
return errors.New(`krew local plugin index is not initialized (run "kubectl krew update")`)
Copy link
Member

@chriskim06 chriskim06 Dec 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think this error message needs to be updated but I'm not exactly sure to what. if we've hit this case that means that there was a non git directory in paths.IndexBase(). my first instinct is to probably fail if we encounter this since we don't want users manually messing around with their krew folders, krew should take care of that. maybe an error like non git directory found in index folder? although I think that's somewhat cryptic. another alternative if we don't want to fail would be to just keep track and make sure that there's at least one git repo in there (not exactly sure if that would cause problems elsewhere, not super likely but also not 100% sure).

cc @ahmetb @corneliusweig for your inputs on this.

Copy link
Contributor Author

@junnplus junnplus Dec 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Can we issue a warning to remind users? like "Invalid index directory"?

Copy link
Member

Choose a reason for hiding this comment

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

sorry this fell off my radar.

I think maybe "invalid index found" is ok. My main problem with this is that there's no additional info for the user if they run into this case. Although I think this is most likely ok since if a user runs into this that means they either manually tampered with their krew directory or there's a bug in krew. If its the latter then hopefully an issue would be filed alerting us.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chriskim06 Thanks your reply, I updated error message. Please take a look.

@junnplus
check all existing index
7fbb7bb 
Signed-off-by: ye.sijun <junnplus@gmail.com>
@junnplus
Copy link
Contributor Author

junnplus commented Jan 7, 2022

@chriskim06 I'm sorry to bother you, do you have any other thoughts on this PR. cc @ahmetb

@junnplus
update error message
35c6981 
Signed-off-by: ye.sijun <junnplus@gmail.com>
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 14, 2022
@ahmetb
Copy link
Member

ahmetb commented Jan 14, 2022

I've manually verified this. It seems to be working fine. I added an extra integration test to ensure this scenario works.
@junnplus sorry to kept you waiting so long.
/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 14, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahmetb, Junnplus

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 14, 2022
@k8s-ci-robot k8s-ci-robot merged commit 0517b50 into kubernetes-sigs:master Jan 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chriskim06 chriskim06 chriskim06 left review comments

@ahmetb ahmetb Awaiting requested review from ahmetb

@soltysh soltysh Awaiting requested review from soltysh

Assignees

@ahmetb ahmetb

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

List fails without default index
4 participants
@junnplus @k8s-ci-robot @ahmetb @chriskim06