New LDAP Auth Method Class #244
Conversation
Codecov Report
@@ Coverage Diff @@
## master #244 +/- ##
==========================================
+ Coverage 86.83% 88.02% +1.18%
==========================================
Files 10 11 +1
Lines 904 977 +73
==========================================
+ Hits 785 860 +75
+ Misses 119 117 -2
|
There was a problem hiding this comment.
I really like the structure of the lib code itself.
The redundancy of unit vs integration tests is a bit cumbersome. The value of having both is clear, but it would be nice (and easier to maintain) if they could be consolidated somehow.
Perhaps a base class or helper(s) that either does or does not set up requests_mock based on whether we're running integration or unit tests? I'm not sure of any other codebases that may have useful patterns to follow in this regard, just thinkin out loud.
| :return: Test certificate contents | ||
| :rtype: str | unicode | ||
| """ | ||
| test_data_dir = os.path.join(os.path.dirname(os.path.realpath(__file__)), '..', '..', '..', '..', '..', 'test') |
There was a problem hiding this comment.
These '..'s smell funny - a helper method somewhere (hvac/tests/utils.py? or a new hvac/tests/test_files.py?) would be nice.
from hvac.tests import test_files
test_cert = test_files.get_file_contents('server-cert.pem')get_file_contents(name) could call a get_filename(name) that joins with a get_base_dir().
removes some boilerplate, and later if test files are moved around, would only need to update get_base_dir().
There was a problem hiding this comment.
@dbwpe: Does 79fc92c cover what you had in mind?
There was a problem hiding this comment.
Then I read your actual comment and bc57308 is closer to the mark...
hvac/api/auth/ldap.py
Outdated
| if policies is None: | ||
| policies = [] | ||
| params = { | ||
| 'policies': ','.join(policies), |
There was a problem hiding this comment.
I always ask myself in cases like this whether I want to enforce input types, or just say "rtfds" (read the frickin docstring). this kind of thing has bitten me in the past:
In [1]: policies = 'only-one-policy'
In [2]: ','.join(policies)
Out[2]: 'o,n,l,y,-,o,n,e,-,p,o,l,i,c,y'If a bunk input type can result in Quite Bad Things, then I'll enforce the type (either with an exception, or help em out and make it a list), otherwise if the API (or whatever) handles it gracefully, just let the chips fall where they may.
There was a problem hiding this comment.
I added a ParamValidation exception a while back for this type of deal, would be reasonable to utilize it here. Could also make the case for casting things to a list if given a string where a list is expected, but I think its better to alert the user to the correct param type rather than hide it away implicitly...
jeffwecan
force-pushed
the
new_ldap_class
branch
from
3480dff
to
bc57308
Compare
Similar idea to #242. Adding in a new "auth" class to cover the Vault LDAP auth method's API routes.
Also related to the spirit of #95.