Closed briantist closed 3 years ago
Merging #128 (8459d0b) into main (c912bf7) will increase coverage by
2.20%
. The diff coverage is95.05%
.
@@ Coverage Diff @@
## main #128 +/- ##
==========================================
+ Coverage 85.76% 87.97% +2.20%
==========================================
Files 29 32 +3
Lines 1082 1247 +165
Branches 83 95 +12
==========================================
+ Hits 928 1097 +169
+ Misses 141 139 -2
+ Partials 13 11 -2
Flag | Coverage Δ | |
---|---|---|
env_docker-default | 87.97% <95.05%> (+2.20%) |
:arrow_up: |
integration | 72.17% <74.19%> (+0.36%) |
:arrow_up: |
py2.6 | 35.02% <32.41%> (-36.78%) |
:arrow_down: |
py2.7 | 79.95% <83.51%> (+1.57%) |
:arrow_up: |
py3.10 | 86.84% <91.75%> (+1.82%) |
:arrow_up: |
py3.5 | 80.27% <83.51%> (+1.52%) |
:arrow_up: |
py3.6 | 80.27% <83.51%> (+1.52%) |
:arrow_up: |
py3.7 | 80.27% <83.51%> (+1.52%) |
:arrow_up: |
py3.8 | 86.84% <91.75%> (+1.82%) |
:arrow_up: |
py3.9 | 86.84% <91.75%> (+1.82%) |
:arrow_up: |
target_auth_none | 100.00% <ø> (ø) |
|
target_auth_token | 73.33% <83.33%> (?) |
|
target_connection_options | 73.78% <ø> (ø) |
|
target_controller | 67.67% <41.02%> (-0.32%) |
:arrow_down: |
target_lookup_hashi_vault | 78.75% <ø> (ø) |
|
target_module_utils | 85.22% <90.10%> (+3.62%) |
:arrow_up: |
units | 84.36% <93.40%> (+3.30%) |
:arrow_up: |
Flags with carried forward coverage won't be shown. Click here to find out more.
Impacted Files | Coverage Δ | |
---|---|---|
plugins/module_utils/_auth_method_aws_iam_login.py | 21.56% <0.00%> (ø) |
|
plugins/module_utils/_auth_method_ldap.py | 57.89% <0.00%> (ø) |
|
plugins/module_utils/_auth_method_userpass.py | 52.38% <0.00%> (+2.38%) |
:arrow_up: |
plugins/module_utils/_authenticator.py | 100.00% <ø> (ø) |
|
...it/plugins/module_utils/authentication/conftest.py | 92.00% <84.61%> (-8.00%) |
:arrow_down: |
plugins/module_utils/_auth_method_token.py | 97.77% <95.83%> (+29.77%) |
:arrow_up: |
plugins/module_utils/_auth_method_approle.py | 89.47% <100.00%> (ø) |
|
plugins/module_utils/_auth_method_jwt.py | 91.30% <100.00%> (-0.37%) |
:arrow_down: |
tests/unit/conftest.py | 100.00% <100.00%> (ø) |
|
...gins/module_utils/authentication/test_auth_none.py | 100.00% <100.00%> (ø) |
|
... and 7 more |
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 c912bf7...8459d0b. Read the comment docs.
Mind filling out the PR template form?
Mind filling out the PR template form?
@webknjaz What information are you looking for that's needed for a tests/internals-only PR?
Mind filling out the PR template form?
@webknjaz What information are you looking for that's needed for a tests/internals-only PR?
Mind filling out the PR template form?
@webknjaz What information are you looking for that's needed for a tests/internals-only PR?
There is no issue type at the moment that applies. I've thought about creating one in the collection for test and CI updates (as I do a lot of them!).
No public components are changed in this PR.
Is there something you would prefer to see in "additional information"?
I'm open to changes here, but a screenshot of empty fields is not really telling me what you want to see. I could delete the headings if that's preferable?
That aside, thank you for the thorough review, I'm going to apply most of your suggestions; much appreciated!
For the issue type, I usually just modify one of the options to be more precise (like Testing Pull Request
or Maintenance Pull Request
).
The component name could be a subpath in the repo. Like tests/
and plugins/module_utils/
.
And additional info could be N/A
.
This all would just make the PR description look way better.
Thanks @webknjaz , I've updated the issue description and I believe I've addressed all your concerns. I've removed the problematic check_import
function and I'll look at better ways to handle that n the future if needed.
SUMMARY
Originally I thought I'd be able to separate out all of the auth methods but it was a bigger job than expected, so this PR is moving
none
andtoken
methods.authenticate
method now expected to return full response, not just the tokentoken
method is special (no actual login happening) since we may not have full response; if we're looking up the token anyway (we usually are), use to it to simulate full login response.ansible-lint
file for integration tests. We're not runningansible-lint
in CI (yet?) but my IDE's ansible plugin is using it, may affect others as well. Nice to have.token
auth to its own integration targettoken
auth tests fromhashi_vault
testshashi_vault
lookup itself; move those tests to their own file within thehashi_vault
target. They are important tests of the plugin's functionality. Eventually, they will be among the only tests remaining there!none
token
ISSUE TYPE
COMPONENT NAME
tests/
ADDITIONAL INFORMATION
N/A