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 verify argument for Client.__init__ #341

Merged
merged 7 commits into from Dec 8, 2022
Merged

Conversation

rolemee
Copy link
Contributor

@rolemee rolemee commented Dec 7, 2022

Turning off ssl verification when using http can save a lot of resources

@codecov-commenter
Copy link

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (30577af) compared to base (6ce40b0).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #341   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           10        10           
  Lines          202       202           
=========================================
  Hits           202       202           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Owner

@sanders41 sanders41 left a comment

Choose a reason for hiding this comment

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

When would this get used? I don’t want to encourage people to not use/verify SSL, but maybe there is a use case I’m not thinking of.

verify needs to be added to the doc string args with a description of how to use it. This is used to auto generate the docs.

meilisearch_python_async/client.py Outdated Show resolved Hide resolved
meilisearch_python_async/client.py Outdated Show resolved Hide resolved
@rolemee
Copy link
Contributor Author

rolemee commented Dec 8, 2022

When using the intranet,meilisearch can be connected by http with api key verifying.The verify argument is used for https,but it's not necessary for http

rolemee and others added 3 commits December 8, 2022 12:22
Co-authored-by: Paul Sanders <psanders1@gmail.com>
Co-authored-by: Paul Sanders <psanders1@gmail.com>
@sanders41
Copy link
Owner

It is possible to use http but would be bad practice. Doing this the api key would be un-encrypted and easy to steal.

@rolemee
Copy link
Contributor Author

rolemee commented Dec 8, 2022

when meilisearch is only open to intranet,https is not necessary.
image
image

the first is verify=True and the second is verify=false.It shows that loading verify module cost much times,but its not used in http request.

@sanders41
Copy link
Owner

Sorry, I missed you said intranet and not internet. That makes sense.

Looks like verify still needs to be added to the doc string. Other than that looks good.

Copy link
Owner

@sanders41 sanders41 left a comment

Choose a reason for hiding this comment

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

Thanks @rolemee

@sanders41 sanders41 merged commit 6338283 into sanders41:main Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants