ansible-collections / community.hashi_vault

Ansible collection for managing and working with HashiCorp Vault.
https://docs.ansible.com/ansible/devel/collections/community/hashi_vault/index.html
GNU General Public License v3.0
81 stars 58 forks source link

Raise error on option duplication in term string #350

Closed vladislav-sharapov closed 1 year ago

vladislav-sharapov commented 1 year ago
SUMMARY

Fixes #349

ISSUE TYPE
COMPONENT NAME

hashi_vault lookup plugin

ADDITIONAL INFORMATION
codecov[bot] commented 1 year ago

Codecov Report

Merging #350 (53219c7) into main (cd881ac) will increase coverage by 0.00%. The diff coverage is 100.00%.

:exclamation: Current head 53219c7 differs from pull request most recent head 996f0a8. Consider uploading reports for the commit 996f0a8 to get more accurate results

@@           Coverage Diff           @@
##             main     #350   +/-   ##
=======================================
  Coverage   98.82%   98.82%           
=======================================
  Files          80       80           
  Lines        4081     4095   +14     
  Branches      258      259    +1     
=======================================
+ Hits         4033     4047   +14     
  Misses         39       39           
  Partials        9        9           
Flag Coverage Δ
env_docker-default 98.82% <100.00%> (+<0.01%) :arrow_up:
integration 81.09% <20.00%> (-0.22%) :arrow_down:
sanity 39.76% <20.00%> (-0.11%) :arrow_down:
target_ansible-doc 100.00% <ø> (ø)
target_auth_approle 89.47% <ø> (ø)
target_auth_aws_iam 50.00% <ø> (ø)
target_auth_azure 53.84% <ø> (ø)
target_auth_cert 86.36% <ø> (ø)
target_auth_jwt 91.30% <ø> (ø)
target_auth_ldap 89.47% <ø> (ø)
target_auth_none 100.00% <ø> (ø)
target_auth_token 71.42% <ø> (ø)
target_auth_userpass 85.71% <ø> (ø)
target_connection_options 74.76% <ø> (ø)
target_controller 83.78% <100.00%> (+0.11%) :arrow_up:
target_filter_vault_login_token 77.77% <ø> (ø)
target_import 39.76% <20.00%> (-0.11%) :arrow_down:
target_lookup_hashi_vault 81.33% <ø> (ø)
target_lookup_vault_ansible_settings 56.00% <20.00%> (-0.28%) :arrow_down:
target_lookup_vault_kv1_get 91.30% <ø> (ø)
target_lookup_vault_kv2_get 91.11% <ø> (ø)
target_lookup_vault_list 90.00% <ø> (ø)
target_lookup_vault_login 88.57% <ø> (ø)
target_lookup_vault_read 90.00% <ø> (ø)
target_lookup_vault_token_create 79.24% <ø> (ø)
target_lookup_vault_write 57.06% <20.00%> (-0.34%) :arrow_down:
target_module_utils 97.35% <ø> (ø)
target_module_vault_kv1_get 87.50% <ø> (ø)
target_module_vault_kv2_delete 56.93% <ø> (ø)
target_module_vault_kv2_get 87.23% <ø> (ø)
target_module_vault_list 85.71% <ø> (ø)
target_module_vault_login 83.72% <ø> (ø)
target_module_vault_pki_generate_certificate 78.72% <ø> (ø)
target_module_vault_read 85.71% <ø> (ø)
target_module_vault_token_create 91.66% <ø> (ø)
target_module_vault_write 56.25% <ø> (ø)
target_modules 81.78% <ø> (ø)
units 96.55% <100.00%> (+0.01%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
plugins/plugin_utils/_hashi_vault_lookup_base.py 100.00% <100.00%> (ø)
.../plugin_utils/base/test_hashi_vault_lookup_base.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

briantist commented 1 year ago

Hi @vladislav-sharapov , I've decided to go with making this a warning for now, and then making the actual breaking change in v5.0.0 so that there's a bit of lead time.

I've pushed some commits to:

Please take a look and let me know if that looks ok to you.

If you'd like to put up a PR for the v5 work too, that'd be fine (it won't merge until around May when we're gearing up to release that version), if not that's fine too, I'll be able to take care of that when it gets closer.

vladislav-sharapov commented 1 year ago

If you'd like to put up a PR for the v5 work too, that'd be fine (it won't merge until around May when we're gearing up to release that version), if not that's fine too, I'll be able to take care of that when it gets closer.

I'm not in a hurry. It's up to you when the PR should be merged.

briantist commented 1 year ago

If you'd like to put up a PR for the v5 work too, that'd be fine (it won't merge until around May when we're gearing up to release that version), if not that's fine too, I'll be able to take care of that when it gets closer.

I'm not in a hurry. It's up to you when the PR should be merged.

With this PR changing to a warning, it will merge as soon as it's ready, I think probably today as long as you're ok with the changes I put up.

We'll need a second PR for the v5 changes; that's the one that will have to wait until closer to that release. I'm fine to handle that one, or you can submit those changes as well (now or leading up to the v5 release) if you'd like to, either way all good.

vladislav-sharapov commented 1 year ago

Thank you for clarifying. I'm OK with the changes. I think you have better understanding how the v5 changes should look like.