-
Notifications
You must be signed in to change notification settings - Fork 30
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
remove kv stores from install.py #156
Conversation
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 think from a maintenance perspective, its better that this is decoupled. If for example etcd reports a critical vulnerability and requests everybody patch, suddenly we find ourself in need of making a release right? I'd like to avoid this situation and think its entirely reasonable that this is offloaded to the jupyterhub admin or user that provides a distribution.
I think this PR requires approval of at least one more person than me to be merged, and it has failing tests currently I see. But, the changes LGTM from what I can tell.
Also with 3.5.7 we keep hanging, so maybe trial 3.4.x now for the purpose of diagnosing, while we should support 3.5.7 also. I'm on a mobile now and will not act. |
Downgrading etcd to 3.4.24 raised the following error in tests: It looks like it's because etcd uses internally the ETCD_VERSION env var etcd-io/etcd#11225 and it's a bool. So, the last commit renames it to ETCD_DOWNLOAD_VERSION. Let's see if this also fixes the hanging part |
Co-authored-by: Erik Sundell <[email protected]>
Thanks! |
As discussed in #155, I don't think we should be handling installation of kv stores. I think it's very rare that installing a kv store is likely to be done via
install.py
, asinstall.py
is useful, a file provider is probably better,