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

added Authorization header if access_token provided #954

Merged
merged 2 commits into from
Sep 20, 2019

Conversation

kaseyhackspace
Copy link
Contributor

This fixes issue #952 by adding an Authorization: token <access_token> header to Github API requests from Binderhub if access_token is passed by either binderhub_config.py or GithubRepoProvider in secret.yaml.

@betatim
Copy link
Member

betatim commented Sep 17, 2019

Thanks for the PR!

I think @sgibson91 has been looking into this as well so getting your input here would be great. I've never even tried to use the features of a BinderHub for "private" deployments so don't have a good feeling if this would break other things or if it should have been solved some other way.

@betatim
Copy link
Member

betatim commented Sep 17, 2019

This change seems to have broken one of the tests here (sorry for the very verbose output of our CI).

@betatim
Copy link
Member

betatim commented Sep 17, 2019

There is also #940 to look at/compare to/check with.

@kaseyhackspace
Copy link
Contributor Author

kaseyhackspace commented Sep 17, 2019

This change seems to have broken one of the tests here (sorry for the very verbose output of our CI).

@betatim I was actually looking into this, and it seems to be a weird point of failure from the CI test. The failure is from a null result of getting gitlab ref. The code changes I made were inside the class GitHubRepoProvider which should not be related to GitlabRepoProvider.

Is there a way to re-trigger the travis-ci tests?

@betatim
Copy link
Member

betatim commented Sep 17, 2019

You think this is a flakey test? I restarted it now. I thought it was related to the change because something in the GitLab provider broke.

@kaseyhackspace
Copy link
Contributor Author

kaseyhackspace commented Sep 17, 2019

You think this is a flakey test? I restarted it now. I thought it was related to the change because something in the GitLab provider broke.

Looked more into this and the failure was also the same as PR #951 so I looked at the actual test. I think we are getting failures because the repository we are testing against was updated from gitlab-ce to gitlab-foss. I did another branch on my fork with an updated repo name and tried a PR there. Waiting for the test to finish to see if that fixes that issue. Will keep you updated here 😄

@kaseyhackspace
Copy link
Contributor Author

@betatim so the ci tests have finished for the new branch I made that rectified the test_gitlab_ref case that was failing and it is now passing (https://travis-ci.com/kaseyhackspace/binderhub/jobs/236599088#L3086). This branch should fix both this PR and the error in PR #951. Question now is, how do we proceed:

@betatim
Copy link
Member

betatim commented Sep 18, 2019

Ah, I just opened #956 to fix the failure after looking at a different PR (but not this one) during my morning coffee. Sorry for the double effort. If you could take a quick look at #956 and leave a review we could merge that. After that I'd rebase this PR and #951 onto master.

@kaseyhackspace
Copy link
Contributor Author

Ah, I just opened #956 to fix the failure after looking at a different PR (but not this one) during my morning coffee. Sorry for the double effort. If you could take a quick look at #956 and leave a review we could merge that. After that I'd rebase this PR and #951 onto master.

It's all good @betatim , it's never a wasted effort 😄 #956 is good to merge.

@betatim betatim merged commit 99af80d into jupyterhub:master Sep 20, 2019
yuvipanda pushed a commit to jupyterhub/helm-chart that referenced this pull request Sep 20, 2019
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.

3 participants