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

Update to Docs: Clarify where in secret.yaml GitHub Personal Access Token should be added #835

Merged
merged 1 commit into from
Apr 29, 2019

Conversation

sgibson91
Copy link
Member

Hi,

This is a tiny PR to update the docs surrounding adding the Personal Access Token for the GitHub API limit to secret.yaml. It wasn't entirely clear that it needs to go under jupyterhub and I got caught out this morning.

Thanks!

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.

@sgibson91 ❤️ lgtm!

@sgibson91 sgibson91 merged commit 970f1c7 into jupyterhub:master Apr 29, 2019
yuvipanda pushed a commit to jupyterhub/helm-chart that referenced this pull request Apr 29, 2019
@sgibson91
Copy link
Member Author

I'm now questioning whether this is correct. My Hub is becoming very problematic when I have this in my secret.yaml 🤔Slow to start after periods of inactivity and often not spinning up user pods. Hub becomes much more responsive when I remove the access token.

@sgibson91
Copy link
Member Author

sgibson91 commented May 3, 2019

I think I've managed to convinced myself that this is incorrect. Going to revert the PR.

Reverting PR #841

@betatim
Copy link
Member

betatim commented May 3, 2019

It seems weird that the hub would become unresponsive with this change. Even if it is "the wrong" way to configure it. We should investigate that.

Whether or not you need the extra level of "indentation" depends on whether you setup your own meta chart (like we do for https://github.com/jupyterhub/mybinder.org-deploy/) or if you just use the binderhub chart.

@sgibson91
Copy link
Member Author

Some kind of check for indentations during helm upgrade would be nice. So many of my problems would have been fixed if I had a message like "this key isn't where I expected it to be so I won't implement it this time".

@sgibson91
Copy link
Member Author

"P.S. This may cause weird Hub behaviour"!

@consideRatio
Copy link
Member

@sgibson91 I've been thinking about these pain points as well without coming up with a concrete idea on how to resolve the pain in a sustainable manner =/

@betatim
Copy link
Member

betatim commented May 3, 2019

Can we do something with helm linting or specifying a flag to the helm executable to say "please shout if there are things defined in values.yaml that aren't consumed in the chart(s)? I think this is super tricky to do but you never know :-/

@sgibson91
Copy link
Member Author

Yeah, other than checking the parents of each key I'm not sure of a good way either 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants