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

remove kv stores from install.py #156

Merged
merged 6 commits into from
Feb 22, 2023
Merged

Conversation

minrk
Copy link
Member

@minrk minrk commented Feb 21, 2023

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, as

  • ~everywhere install.py is useful, a file provider is probably better,
  • ~everywhere a kv store is desirable, an official container image is more likely how these will be installed, not in the same container as the Hub
  • installing go binaries isn't complex for our target users, and both tools have simple copy/paste installs

Copy link
Member

@consideRatio consideRatio left a 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.

docs/source/install.md Outdated Show resolved Hide resolved
@consideRatio
Copy link
Member

consideRatio commented Feb 22, 2023

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.

@GeorgianaElena
Copy link
Member

Downgrading etcd to 3.4.24 raised the following error in tests:
etcdmain: error verifying flags, invalid value "3.4.24" for ETCD_VERSION: parse error. See 'etcd --help'.

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 <erik.i.sundell@gmail.com>
@minrk minrk merged commit 011559c into jupyterhub:main Feb 22, 2023
@minrk minrk deleted the rm-install-kv branch February 22, 2023 11:33
@minrk
Copy link
Member Author

minrk commented Feb 22, 2023

Thanks!

@minrk minrk added maintenance api-change breaking changes and removed maintenance labels Mar 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-change breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants