Skip to content
This repository has been archived by the owner on Mar 1, 2024. It is now read-only.

feat: allow use of self-signed cert for MinIO server with MinioReader #935

Merged

Conversation

ferdinandosimonetti
Copy link
Contributor

@ferdinandosimonetti ferdinandosimonetti commented Feb 8, 2024

Description

I've added cert_check as an option for MinioReader to avoid problems when the remote MinIO server has TLS enabled, but with a self-signed or untrusted certificate.
How can I run my modified version of MinioReader locally, to perform some test?

Type of Change

Please delete options that are not relevant.

  • Bug fix / Smaller change

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Added new unit/integration tests
  • Added new notebook (that tests end-to-end)
  • I stared at the code and made sure it makes sense

Suggested Checklist:

  • I have added a library.json file if a new loader/tool was added
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I ran make format; make lint to appease the lint gods

Copy link
Collaborator

@anoopshrma anoopshrma left a comment

Choose a reason for hiding this comment

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

Everything looks great! Thankyou for the contribution @ferdinandosimonetti.

Linting is missing in here if you you could fix that up we are good for merge

Copy link
Collaborator

@anoopshrma anoopshrma left a comment

Choose a reason for hiding this comment

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

Added one comment

@@ -12,6 +12,7 @@
from llama_index.readers.base import BaseReader
from llama_index.readers.schema.base import Document

import urllib3
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move this import inside load_data method. We follow importing external libraries inside the class to avoid any build failure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@anoopshrma
Copy link
Collaborator

Try running make format; make lint to remove any linting issue

@ferdinandosimonetti
Copy link
Contributor Author

Here we are...

ferdi@u820:~/llama-hub$ python -m venv ~/testing-llamahub
ferdi@u820:~/llama-hub$ . ~/testing-llamahub/bin/activate
(testing-llamahub) ferdi@u820:~/llama-hub$ pip install -r test_requirements.txt
(testing-llamahub) ferdi@u820:~/llama-hub$ pip install black[jupyter]
(testing-llamahub) ferdi@u820:~/llama-hub$ make format
black .
All done! ✨ 🍰 ✨
694 files left unchanged.
(testing-llamahub) ferdi@u820:~/llama-hub$ make lint
ruff check .
black --check .
All done! ✨ 🍰 ✨
694 files would be left unchanged.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@anoopshrma
Copy link
Collaborator

Finally you conquered the linting god @ferdinandosimonetti 😆

Thank you for the quick response on the PR

@anoopshrma anoopshrma merged commit ec27ff1 into run-llama:main Feb 9, 2024
3 checks passed
@ferdinandosimonetti ferdinandosimonetti deleted the feat/cert_check-for-MinioReader branch February 12, 2024 14:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants