Closed briantist closed 2 years ago
Merging #267 (4088b32) into main (e903e3b) will increase coverage by
0.06%
. The diff coverage is100.00%
.:exclamation: Current head 4088b32 differs from pull request most recent head 6b23da8. Consider uploading reports for the commit 6b23da8 to get more accurate results
@@ Coverage Diff @@
## main #267 +/- ##
==========================================
+ Coverage 98.34% 98.40% +0.06%
==========================================
Files 66 68 +2
Lines 3208 3331 +123
Branches 267 288 +21
==========================================
+ Hits 3155 3278 +123
Misses 44 44
Partials 9 9
Flag | Coverage Δ | |
---|---|---|
env_docker-default | 98.40% <100.00%> (+0.06%) |
:arrow_up: |
integration | 82.16% <98.73%> (+1.12%) |
:arrow_up: |
sanity | 39.27% <31.64%> (-0.62%) |
:arrow_down: |
target_ansible-doc | 100.00% <ø> (ø) |
|
target_auth_approle | 89.47% <100.00%> (ø) |
|
target_auth_aws_iam | 50.00% <100.00%> (ø) |
|
target_auth_cert | 86.36% <100.00%> (ø) |
|
target_auth_jwt | 91.30% <100.00%> (ø) |
|
target_auth_ldap | 89.47% <100.00%> (ø) |
|
target_auth_none | 100.00% <100.00%> (ø) |
|
target_auth_token | 73.07% <100.00%> (+1.64%) |
:arrow_up: |
target_auth_userpass | 85.71% <100.00%> (ø) |
|
target_connection_options | 74.76% <ø> (ø) |
|
target_controller | 81.90% <95.03%> (+1.05%) |
:arrow_up: |
target_filter_vault_login_token | 77.77% <ø> (ø) |
|
target_import | 39.27% <31.64%> (-0.62%) |
:arrow_down: |
target_lookup_hashi_vault | 81.33% <ø> (ø) |
|
target_lookup_vault_ansible_settings | 57.63% <92.40%> (?) |
|
target_lookup_vault_kv1_get | 91.30% <ø> (ø) |
|
target_lookup_vault_kv2_get | 91.11% <ø> (ø) |
|
target_lookup_vault_login | 88.57% <ø> (ø) |
|
target_lookup_vault_read | 90.00% <ø> (ø) |
|
target_lookup_vault_token_create | 78.18% <ø> (ø) |
|
target_lookup_vault_write | 59.43% <92.59%> (+0.29%) |
:arrow_up: |
target_module_utils | 97.03% <94.54%> (-0.21%) |
:arrow_down: |
target_module_vault_kv1_get | 87.23% <ø> (ø) |
|
target_module_vault_kv2_get | 86.95% <ø> (ø) |
|
target_module_vault_login | 83.33% <ø> (ø) |
|
target_module_vault_pki_generate_certificate | 78.26% <ø> (ø) |
|
target_module_vault_read | 85.36% <ø> (ø) |
|
target_module_vault_token_create | 90.00% <ø> (ø) |
|
target_module_vault_write | 57.88% <92.59%> (+0.35%) |
:arrow_up: |
target_modules | 78.90% <81.48%> (-0.18%) |
:arrow_down: |
units | 95.43% <100.00%> (+0.17%) |
:arrow_up: |
Flags with carried forward coverage won't be shown. Click here to find out more.
Impacted Files | Coverage Δ | |
---|---|---|
plugins/doc_fragments/auth.py | 100.00% <ø> (ø) |
|
plugins/lookup/vault_login.py | 100.00% <ø> (ø) |
|
plugins/modules/vault_login.py | 100.00% <ø> (ø) |
|
plugins/lookup/vault_ansible_settings.py | 100.00% <100.00%> (ø) |
|
plugins/module_utils/_auth_method_approle.py | 89.47% <100.00%> (ø) |
|
plugins/module_utils/_auth_method_aws_iam.py | 94.23% <100.00%> (ø) |
|
plugins/module_utils/_auth_method_cert.py | 95.45% <100.00%> (ø) |
|
plugins/module_utils/_auth_method_jwt.py | 95.65% <100.00%> (ø) |
|
plugins/module_utils/_auth_method_ldap.py | 89.47% <100.00%> (ø) |
|
plugins/module_utils/_auth_method_none.py | 100.00% <100.00%> (ø) |
|
... and 17 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 e903e3b...6b23da8. Read the comment docs.
I used
deprecated_features
but I don't think it's quite right. Nothing is being deprecated, just changed.
The old behavior of using the old default value is deprecated. So IMO this is ok :)
I would also add some code changes though so that users are informed that if they don't specify the value explicitly they will have a breaking change.
I used
deprecated_features
but I don't think it's quite right. Nothing is being deprecated, just changed.The old behavior of using the old default value is deprecated. So IMO this is ok :)
Thanks! Ok I'll leave it.
I would also add some code changes though so that users are informed that if they don't specify the value explicitly they will have a breaking change.
I was thinking about this. If I warn all that time that will be pretty annoying.
Unfortunately, I can't tell easily when the value was gotten from defaults, unless I use the same type of config manager code I used in the settings lookup.
Actually, just running that lookup from within other plugins would be an easy way to warn selectively without (re)writing a bunch of code.. it would only work in plugins though.
In modules, I guess I could change the true default to None and then set it after I know.. i think sanity would not like it unless i change the docs too which is not great.
What do you think @felixfontein ?
I did reply on IRC, for sake of completeness I'll repost it here:
the 'common way' is to remove the default and in case the value is None, issue a deprecation warning and manually set the default value you can't use
default: xxx
in DOCUMENTATION though for modules (due to sanity tests), but you can't do that for plugins either, so the common thing is to mention this in in the regular description of the option
Thank you for contribution!✨
This PR has been merged and the docs are now incorporated into main
:
https://ansible-collections.github.io/community.hashi_vault/branch/main
Thank you for reviewing @felixfontein !
SUMMARY
Related: #248
Announce that the default value for
token_validate
is changing in 4.0.0.Will repeat this PR in the release for 3.0.0 so that it's announced in the porting guide for 4.0.0 too.
Which changelog section?
I used
deprecated_features
but I don't think it's quite right. Nothing is being deprecated, just changed.I'm trying to consider if
breaking_changes
is more appropriate, either now for the announcements, in 4.0.0 for the actual change, or even both?major_features
feels too major. For the actual change,removed_features
doesn't really make sense.ISSUE TYPE