-
Notifications
You must be signed in to change notification settings - Fork 29
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
Make etcd3 & python-consul2 soft dependencies #127
Conversation
Thanks for submitting your first pull request! You are awesome! 🤗 |
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.
Not installing these deps for TraefikToml seems like a good idea 👍 Thanks @yuvipanda!
@@ -1,7 +1,5 @@ | |||
aiohttp | |||
escapism | |||
etcd3 |
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.
Do you want to add these (etcd3 and python-consul2) to dev-requirements.txt
instead, to fix the tests?
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.
That's not a bad idea at all! This is a breaking change tho, so we should announce widely.
I also want to test that the toml provider works without these libraries. How do you think we can do that?
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 didn't notice any toml proxy test failing in https://github.com/jupyterhub/traefik-proxy/pull/127/checks?check_run_id=2211574725 ❤️ Only the consul and etcd ones.
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.
Right. But in the future, if we accidentally introduce a change that automatically imports etcd3 or python-consul2, the tests in this repo won't catch it! But for an end user who doesn't have these packages installed but only wants to use the TOML proxy, it will fail. So I want to find a way to test that the toml proxy works even without these packages installed.
Prevents CI from failing
Hello. Any updates here? |
Thanks for the ping @dolfinus! This is ready to go, we just need to document the change somewhere visible and add a test to ensure we won't introduce any change in the future that might break TomlProxy when etcd3 and consul clients aren't installed (see @yuvipanda's comment above). I'll open a new issue about this to un-block this PR. |
Thanks! |
Fixes #125
TODO: