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

Allow git_credentials to be configurable #823

Merged
merged 2 commits into from
Apr 11, 2019

Conversation

katylava
Copy link
Contributor

@katylava katylava commented Apr 9, 2019

This will make it so repo2docker can clone private GitLab repos as long as GitLabRepoProvider.git_credentials is configured.

I have no way of testing that this will work with an actual secret.yaml file. I believe there is more work to do for it to read this value from a yaml file -- in the helm-chart templates. Can someone confirm this for me?

Also the tests fail for me on master, probably because I'm missing some step I need in order to run them. The contributing docs just say to run pytest, but it seems there's more to it than that. Anyway, I can't test this.

There are a couple other ways I could do this:

  • Only make it configurable in GitLabRepoProvider, instead of in all repo providers via the RepoProvider base class.
  • Create a new configurable value, like git_username, and build the credential string in a method on GitLabRepoProvider (since access_token is already configurable and that's the git password).

I'm not sure what way is best. Please provide feedback.

@meeseeksmachine
Copy link

This pull request has been mentioned on Jupyter Community Forum. There might be relevant details there:

https://discourse.jupyter.org/t/how-do-you-configure-binder-when-running-locally-with-minikube/666/10

@betatim
Copy link
Member

betatim commented Apr 10, 2019

Thanks for following up with this PR!

After (re)reading our code for a while and pondering why it is the way it is I think for the GitLab repo provider we should copy the GitHub one:

@default('git_credentials')
def _default_git_credentials(self):
if self.access_token:
# Based on https://github.com/blog/1270-easier-builds-and-deployments-using-git-over-https-and-oauth
# If client_id is specified, assuming access_token is personal access token. Otherwise,
# assume oauth basic token.
if self.client_id:
return r'username={client_id}\npassword={token}'.format(
client_id=self.client_id, token=self.access_token)
else:
return r'username={token}\npassword=x-oauth-basic'.format(token=self.access_token)
return ""

but change the comment and format of the git credentials string it produces. This would save people from having to put their access tokens in two places in the config. From a bit of googling it seems the username can be set to anything and the token goes in the password field (unlike GitHub where it is the other way around ...) So git clone https://mybinderhubuser:<private token>@git.example.com/myuser/myrepo.git should work. Wondering if we should hardcode binderhub as the username.

I think the reason that git_credentials doesn't have the config=True set by default is that it was/is intended to be used as it is in the GitHub repo provider. This means that users don't actually set it themselves but it is computed from the access token.

Regarding setting these value from a helm config file: this should be automatic if you set the BinderHub.config key in the yaml file. We use this for mybinder.org config here

@katylava
Copy link
Contributor Author

Ok, I've updated this according to feedback, and updated the docs as well. I decided to use the private_token config instead of access_token since the GitLab documentation says private_token. I don't know what the difference is otherwise.

I have manually tested this.

@betatim
Copy link
Member

betatim commented Apr 11, 2019

Looks good to me. Merging!

Congratulations on your first merged PR to BinderHub and thanks for the patience :)

Maybe some day someone who knows about access vs private token will stop by and explain it to us.

@betatim betatim merged commit d52e51e into jupyterhub:master Apr 11, 2019
yuvipanda pushed a commit to jupyterhub/helm-chart that referenced this pull request Apr 11, 2019
@katylava
Copy link
Contributor Author

I learned something -- so, private tokens were a thing that was deprecated, but ?private_token=<token> is still what's expected in API calls:

To make authenticated requests to the API, use either the private_token parameter or the Private-Token header
https://docs.gitlab.com/ee/user/profile/personal_access_tokens.html#personal-access-tokens

Technically it's an access_token, but it still has to be called private_token.

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.

4 participants