-
Notifications
You must be signed in to change notification settings - Fork 372
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
cmd: Add 'index remove' command #552
Conversation
/assign @corneliusweig |
042f6e4
to
054331f
Compare
Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks quite good. There are some smaller nits.
And I'd like to discuss how we should treat the --force
option. Its very existence is proof that this is supported behavior. If anything, I would prefer talking about "not recommended". Anyway, we should think hard if we really want that. My opinion is -- either support it -- or don't. Right now, we are going both ways which will create problems, because users will make use of --force
get lost.
So either we hide --force
for the time being, or we make sure that everything else works just fine, if additional plugins are installed. (This will also help us plugin reviewers, just saying :) )
First of all we expect >95% users won't use Here's my primary rationale for having
|
Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
Yeah, that all makes sense. I'm also ok with having So in a way we do support the core workflow after |
Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
I think there's no reason for us to support all those weird corner cases at the moment. We'll make |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
Nice work!
|
||
It is only safe to remove indexes without installed plugins. Removing an index | ||
while there are plugins installed will result in an error, unless the --force | ||
option is used ( not recommended).`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
option is used ( not recommended).`, | |
option is used (not recommended).`, |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahmetb, corneliusweig 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 |
Fixes #550
Related issue: #545, #483