-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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 fuzziness to knife ssh
queries
#3536
Conversation
@@ -204,6 +207,12 @@ def search_nodes | |||
list | |||
end | |||
|
|||
def fuzzify_query | |||
if @query !~ /:/ | |||
@query = "tags:*#{@query}* OR roles:*#{@query}* OR fqdn:*#{@query}* OR addresses:*#{@query}*" |
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.
Should this be "weaker"? Possibilities:
- Remove asterisks for both sides.
- fqdn only? (would seem to align with expectation of using
ssh
) - Keep as is, but have a confirmation prompt for what fqdn we're running on. (Can be explicitly skipped with
--yes
flag.)
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.
It needs to be the same as knife search node.
👍 |
Also, my ruby force is weak here -- any suggestions on how I could pull |
Also, went searching for tests that cover this part, but doesn't seem we test this level, so given that I'm not comfortable enough with starting my own test approach from scratch, I think this is just waiting on feedback on the above, then it's good to merge as far as I'm aware. |
So, what I'd really like to see is knife ssh and knife search node share exactly the same code and call the same method that does fuzziness. Eliminate code duplication and eliminate the chances that we patch in one place and not the other. |
We can't merge this without considering the problem holistically, so 👎 for now. |
Agree, and for the future the Manage code has a similar fuzzifier and should make sure that uses the same algo. |
Totally fair. Closing for now, since the other issue is open |
PR for #3526.