Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

common: Handle aliases correctly #71

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Akasurde
Copy link
Member

SUMMARY

Fixed get_api_client API to handle aliases that
are passed via inventory configuration or module parameters.

Signed-off-by: Abhijeet Kasurde akasurde@redhat.com

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

changelogs/fragments/honor_aliases.yml
molecule/default/tasks/full.yml
plugins/module_utils/common.py

Fixed get_api_client API to handle `aliases` that
are passed via inventory configuration or module parameters.

Signed-off-by: Abhijeet Kasurde <akasurde@redhat.com>
@codecov
Copy link

codecov bot commented Apr 20, 2021

Codecov Report

Merging #71 (8985fc5) into main (b68bf7c) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main      #71   +/-   ##
=======================================
  Coverage   23.17%   23.17%           
=======================================
  Files           1        1           
  Lines         151      151           
  Branches       24       24           
=======================================
  Hits           35       35           
  Misses        111      111           
  Partials        5        5           

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 b68bf7c...8985fc5. Read the comment docs.

@gravesm
Copy link
Member

gravesm commented Apr 20, 2021

I don't understand, shouldn't ansible already handle this? If I pass ssl_ca_cert and then do module.params.get('ca_cert'), won't it already work without this change? If I remove your changes, the tests still pass.

Copy link
Contributor

@abikouo abikouo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand, shouldn't ansible already handle this? If I pass ssl_ca_cert and then do module.params.get('ca_cert'), won't it already work without this change? If I remove your changes, the tests still pass.

I don't understand, shouldn't ansible already handle this? If I pass ssl_ca_cert and then do module.params.get('ca_cert'), won't it already work without this change? If I remove your changes, the tests still pass.

I think using module it should work, assuming the core engine will convert argument from aliases to true name
but for inventory plugin, there is no such a conversion, so this has to be done manually.
We should add some inventory test cases

@@ -10,6 +10,20 @@
debug:
var: output

- name: Use aliases and check if those values are picked up
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we may need to cover all aliases cert_file, key_file, verify_ssl

@@ -131,6 +131,12 @@ def _raise_or_fail(exc, msg):
for true_name, arg_name in AUTH_ARG_MAP.items():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest some validation step to avoid having both alias and true name set as input, otherwise the behavior of the api will be undefined. for instance using

k8s:
  ...
  validate_certs: yes
  verify_ssl: no

the process will the k8s configuration item with the first value in alpha num ?

@fabianvf
Copy link
Contributor

note: AUTH_ARG_MAP doesn't define aliases for ansible, it's used for mapping the Ansible keys to the proper property names in the kubernetes client. It was added because things like ssl_ca_cert were normalized across ansible modules, but that made it so that we couldn't just iterate over the auth_args and set the properties in the kubernetes.Configuration like we were before the normalization. So the true_name doesn't need to be exposed to the user, we just need to know what property in the kubernetes.Configuration object the keys that ansible is giving us map to.

Typing this all out, I'm feeling like maybe I should have left this as a comment way back when I added it 😅

@gravesm
Copy link
Member

gravesm commented Feb 1, 2022

@Akasurde What do you think about adding some inventory test cases?

softwarefactory-project-zuul bot pushed a commit that referenced this pull request Aug 23, 2022
…tions (#500)

Honor aliases for lookup and inventory plugins

rebase and extend the following PR #71
ISSUE TYPE


Bugfix Pull Request

Reviewed-by: Mike Graves <mgraves@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants