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

Seed' apiTokens #2312

merged 3 commits into from
Jul 20, 2021


Copy link

@consideRatio consideRatio commented Jul 13, 2021

Closes #2308 by automatically seeding entries that doesn't have apiToken or api_token set, but reuse explicitly set tokens if they are explicitly set.

Review notes

  • Agree to introduce non-essential breaking change motivated below?
  • Agree to always generate and set an api_token that becomes exposed via the hub k8s Secret?
    Note that JupyterHub 1.4.0 introduced a regression that forces us to set it that has now been fixed in the main branch, but it isn't yet released. If we always generate a token, this will avoid problems with that. See Fix regression where external services api_token became required jupyterhub#3531.
  • Does the implementation look ok?
  • Is it documented ok? (PR preview here)

About non-essential breaking change introduced

I've introduced an isolated commit (925a4a6) with a very minor breaking change that is unlikely to influence people. The motivation is that it will be the name I think people expect as well as it will be something far more consistent in its naming by actually being the path to a certain value in the config.

It is a breaking change if both the following conditions apply, which I find unlikely and not merit a 2.0.0 release (but 1.1.0) and still worth doing to reduce complexity etc.

  1. You have configuration like this:
        # potentially breaking: you have a config key different from the name like below
          name: my-service-name
  2. You reference the hub k8s Secret's key from something external to the JupyterHub Helm chart itself, previously named, but is now named

@consideRatio consideRatio force-pushed the pr/seed-api-tokens branch 2 times, most recently from 504a00f to 9170751 Compare July 13, 2021 15:58
@consideRatio consideRatio changed the title Seed apiTokens Seed' apiTokens Jul 13, 2021
Copy link

@yuvipanda yuvipanda left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, @consideRatio.. I think the breaking change is fine and doesn't need a major version bump.

I'll leave this open for a few more days for others to review. I think this will be great for BinderHub!

Copy link
Member Author

Thanks for your review @yuvipanda, does it feel okay to go for a merge at this point?

Copy link
Member Author

I did a final self-review of this and it LGTM. I consider this to be evaluated well enough to be merged at this point.

@consideRatio consideRatio merged commit 417e84e into jupyterhub:main Jul 20, 2021
consideRatio pushed a commit to jupyterhub/helm-chart that referenced this pull request Jul 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet

Successfully merging this pull request may close these issues.

Autogenerate and set passwords for JupyterHub's registered services' api_tokens
2 participants