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

Enable github private repos. #429

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jgoppert
Copy link

@jgoppert jgoppert commented Jun 16, 2017

I'm making a private build farm, and I've seen a lot of people doing local clones and file:/// references for their ROS_DISTRO_URL and this was just so horribly clunky I fixed it.

This could probably be better intertwined with the .config/bloom settings. I had to set both the token in .config/bloom and GITHUB_PASSWORD to my github "Persontal Access Token" to get it to work.

This also supports private automatic PR's from bloom!

Needs: ros-infrastructure/rosdistro#103

@nuclearsandwich
Copy link
Contributor

Does python's http client support .netrc transparently? If not, could you parse auth data from a .netrc file so that this supports more than just GitHub?

@jgoppert
Copy link
Author

jgoppert commented Jun 16, 2017

I just hacked this together from the approach I saw when digging through rosdistro. It would be great if netrc worked.

After an initial look, maybe .netrc would work if we switched to the requests package for everything, but would still be nice to get this in as a stop-gap until someone has the time to do that: https://stackoverflow.com/questions/2018026/what-are-the-differences-between-the-urllib-urllib2-and-requests-module

If there is no fall-out from just replacing all the calls I would be willing to do that.

Copy link
Contributor

@nuclearsandwich nuclearsandwich left a comment

Choose a reason for hiding this comment

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

What python versions was this tested across?

I notice that a lot of the explicit utf-8 encodings have been dropped for dictionary keys and tag contents. Can you explain why this was needed?

@nuclearsandwich
Copy link
Contributor

@jgoppert apologies for leaving this by.

I think netrc is a general enough interface to extend to providers beyond GitHub and would be a valuable addition to rosdistro as well. But I don't want to let the ideal stand in the way of a contribution that can be useful to folks today.

It looks like @dirk-thomas was involved in the GitHub authentication support for rosdistro. Do you or @wjwwood have any feelings on whether a similar approach is suitable for Bloom?

@wjwwood
Copy link
Contributor

wjwwood commented Aug 25, 2017

I've tried to avoid bloom storing any credentials that aren't api keys. netrc is attractive because it is completely out of band with our tool. Also most people use ssh keys and git's insteadOf config to automatically convert https git remote urls to ssh instead.

@jgoppert
Copy link
Author

jgoppert commented Sep 3, 2017

@wjwwood Is there any way to fix the problem below, from (ros-infrastructure/rosdistro#103) ?

If there is an alternative to using https credentials, such as git+ssh that doesn't require downloading your rosdistro and ros_buildfarm_config locally with a file:/// url, let me know. But I have tried just putting the access tokens in the urls and that doesn't work with https://TOKEN@raw.githubusercontent.com/PRIVATE_REPO/rosdistro/master/index.yaml.

This is what motivated me to create this PR and if it can be resolved by using insteadOf or something similar that would be much easier. I don't want to maintain this so I would either like to find a way to get it merged or find an alternative.

@nuclearsandwich
Copy link
Contributor

If you pulled git-credential-netrc from git contrib, can you use for authentication without involving Bloom or rosdistro at all?

@jgoppert
Copy link
Author

jgoppert commented Sep 3, 2017

@nuclearsandwich git works fine with ssh keys. The issue is that you can't download the rosdistro file from a private repository on github using netrc. I just tried using wget + netrc and that doesn't work with an url like: https://raw.githubusercontent.com/PRIVATE_REPO/rosdistro/master/index.yaml. componentjs/remotes.js#19 might be related.

@nuclearsandwich
Copy link
Contributor

My apologies for misunderstanding where the auth breakdown is.

Okay, so reading some wget docs it looks like wget will only attempt to use authentication after receiving a 401 status. Because GitHub returns 404s for unauthorized repository content requests in order to avoid leaking filenames wget with netrc won't do the trick. The request does work with curl --netrc.

I think some of the challenge is that two different teams are reviewing ros-infrastructure/rosdistro#103 and this PR. I don't have the bandwidth before ROSCon to get back up to speed with the discussion in that PR so from my perspective if that PR merges, bloom can probably take the changes here with some refinements which I thought I'd already commented on but GitHub must've tried to start a review instead of standalone comments and they got lost. I'll put those back up later today.

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

Successfully merging this pull request may close these issues.

None yet

3 participants