-
Notifications
You must be signed in to change notification settings - Fork 88
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
Support authentication #128
base: master
Are you sure you want to change the base?
Conversation
0a7b055
to
a9e9a4f
Compare
d341bc2
to
a9e9a4f
Compare
Huh. I'm not sure what I'm doing wrong on the mock bits for earlier pythons. |
ffd89d0
to
95b8db6
Compare
Ah ha, it's an upstream python bug. All fixed now. |
7a1be83
to
042fc24
Compare
As discussed offline it needs to be double checked that this contribution complies with the license. |
`vcs_base.load_url()` currently doesn't support authentication. Add support for both basic and token-based authentication by parsing netrc-formatted files. Use `appdirs` to support vcstool-specific authentication files for both the user and the system (user takes precedence). Signed-off-by: Kyle Fazzari <kyle@canonical.com>
042fc24
to
b7f27ae
Compare
Alright, this has been completely redesigned and rewritten from scratch. It does introduce the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add some documentation about the new auth files to the README.
(I haven't looked at the test code yet.)
Signed-off-by: Kyle Fazzari <kyle@canonical.com>
Signed-off-by: Kyle Fazzari <kyle@canonical.com>
Signed-off-by: Kyle Fazzari <kyle@canonical.com>
b494569
to
91f6ad0
Compare
Signed-off-by: Kyle Fazzari <kyle@canonical.com>
Signed-off-by: Kyle Fazzari <kyle@canonical.com>
Signed-off-by: Kyle Fazzari <kyle@canonical.com>
Signed-off-by: Kyle Fazzari <kyle@canonical.com>
Signed-off-by: Kyle Fazzari <kyle@canonical.com>
Signed-off-by: Kyle Fazzari <kyle@canonical.com>
8b402e4
to
fc361a2
Compare
Done in fc361a2, does that seem to be a sensible place to put it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a few minor edits myself but added some more questions / suggestions inline.
if e.errno not in (errno.ENOENT, errno.EACCES): | ||
raise | ||
except netrc.NetrcParseError: | ||
# If this file had issues, don't error out so we can try fallbacks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would a warning be more helpful to the user rather then silently ignoring it? On the other hand this behavior might match how other programs using .netrc
work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're a little bit in uncharted territory here, to be honest. My inspiration for the design is apt
, but one generally runs that as root, so running into permissions issues accessing those files isn't normally an issue. It could be here, if the machine admin only wanted members of a particular group to be able to fetch repositories using these credentials. I had mixed feelings about warning about this or quietly proceeding, so I'm happy adding a warning if you like.
Signed-off-by: Kyle Fazzari <kyle@canonical.com>
For python 2 compatibility. Signed-off-by: Kyle Fazzari <kyle@canonical.com>
Gentle ping here. |
Hey @dirk-thomas, how are you feeling about this? |
@kyrofa / @dirk-thomas is there a reason why this MR was not accepted? |
vcs_base.load_url()
currently doesn't support authentication. Add support for both basic and token-based authentication by parsing netrc-formatted files. Useappdirs
to support vcstool-specific authentication files for both the user and the system (user takes precedence).