-
Notifications
You must be signed in to change notification settings - Fork 8
Improve the ElasticSearch cluster configuration to allow basic_auth ES access #267
base: master
Are you sure you want to change the base?
Conversation
'port': int(port), | ||
'use_ssl': use_ssl, | ||
'http_auth': http_auth | ||
}) |
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.
Isn't there any python lib that could handle this for us?
If not, I suggest you could at least move this piece of code to some utility function
Updated. |
then \ | ||
test "$$(grin " $$" daybed/ docs/ -l | wc -l)" -ne "0" && \ | ||
grin -l " $$" daybed/ docs/ | xargs sed -i 's/\s*$$//' || exit 0; \ | ||
fi |
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.
I don't really see the point of this here, you could define it in your shell instead, no ?
(.phony is repeated, is that ok ?)
I'm not against merging this, but a bit doubtful though. We can have all this code in Daybed, but in the mean time, opening a PR on elasticsearch-py would be worth it IHMO. I don't see why it shouldn't support absolute urls for hosts! |
Why should it? |
The dict formalism is rather curious, since there's a standard for that! RFC-1738 :) |
I can make a pull-request to them based on this assumption. There is not urgency yet to merge this patch since I already applied it in production. |
d28e89b
to
dcc5fab
Compare
Ok. Also, I wouldn't have created a dedicated module just for one utility function. But it may be a matter of taste :) |
Exactly the reason why I did it like so :) |
Here we go: elastic/elasticsearch-py#149 |
Good job @Natim! Your PR was merged in ES python client :) We will soon be able to close this here! |
No description provided.