Codecov Report

Merging #246 into master will increase coverage by 0.05%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #246      +/-   ##
==========================================
+ Coverage   95.79%   95.85%   +0.05%     
==========================================
  Files           3        3              
  Lines         571      579       +8     
  Branches      123      125       +2     
==========================================
+ Hits          547      555       +8     
  Misses         15       15              
  Partials        9        9

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df40891...b5e9b48. Read the comment docs.

The test success is a bit deceptive. Since we with VCR make requests work even if no or wrong credentials are given, the netrc test succeeds as well. But maybe that is actually OK, since this is what we do for all tests.

👍 Maybe instead of trying to access a .netrc file in the home directory in tests we can get by with just mocking the netrc file access as is done in the unit tests of requests itself?

Good thinking, @valgur. Mocking requests here does not work, though, because I read the netrc file already in the CLI function.

I could follow the same logic and monkey-patch the netrc class here: https://github.com/python/cpython/blob/master/Lib/netrc.py#L22-L30, overriding the __init__ function so it does not read the file. That is easy, but maybe a bit dirty. But maybe still better than my solution here and the netrc module will probably not change to much.

Before I do anything - do you support my choice of doing the parsing in the CLI to avoid confusion deeper down (also @Fernerkundung)?

My concern is that if we allow omitting user and password in the SentinelAPI initialization, some users might think authorization is not required. But we could also circumvent that by making it a "power-user feature", where user and password still do not have defaults on the function but you can explicitly set them both to None if you know that you have a ~/.netrc file in place.

• Pro: That would greatly reduce the amount of extra code for this feature.
• Con: To make this safe in the CLI, we would perhaps need to add an extra flag --use-netrc so omitting -u and -p without netrc still fails early.

I think adding the .netrc support to the Python API by using it when both user and pass are set to None would be a good approach. Since using credentials directly in a script is something you want to avoid just like on the CLI I think it would be a good addition. It leaves no potential for confusion or weird corner cases since setting them to None has no other conceivable meaning or purpose. Maybe we should add environment variable support to the API as well in that case (so we first try to get credentials from parameters, then envvars, then .netrc)?

Regarding the implementation, I suggest replacing the custom _try_netrc_auth with requests.sessions.get_netrc_auth(self.api_url). This means that we can skip adding tests for actually parsing the .netrc file and rely on the requests' implementation.

do you support my choice of doing the parsing in the CLI to avoid confusion deeper down

👍

Maybe we should add environment variable support to the API as well in that case (so we first try to get credentials from parameters, then envvars, then .netrc)?

In that case how would a user know whether the API takes the envvar or the .netrc file? From a design perspective I would favour it if setting user/pass to None results in only one location the credentials are taken from.

Do you think we should also include a way to explicitly set the path to a .netrc file? A use case for that would be, if permissions on a DHuS instance are linked to user accounts and one user has to have multiple accounts, depending on the scope of the query they perform.

I suggest replacing the custom _try_netrc_auth with requests.sessions.get_netrc_auth(self.api_url)

Good idea. Done. I don't think we can drop the tests, though, and they work fine as they are. So maybe we are good?

From a design perspective I would favour it if setting user/pass to None results in only one location the credentials are taken from.

I agree.

Actually, this already works:

SentinelAPI(None, None).query(uuid='eaa093dd-46a6-49b5-a1bd-7625c2956029')

So all we need is to add this to the docs.

Do you think we should also include a way to explicitly set the path to a .netrc file?

No, let us not do that. The requests parsing function does not support it and if someone has that a special use pattern, they can find other ways. Since the entries in the .netrc file are organized by hostname, you can have different credentials for different DHuS, e.g. a pre-ops hub etc.

So we actually support .netrc both in the CLI and in the API now with very little extra code. I'd consider this done and just add a few lines in the docs. Is that okay?