Introduce ANSIBLE_PLAYBOOK_EXTRA_ARGS #215

kshtsk · 2020-10-28T16:15:47Z

This change makes it possible to add extra variables
for ansible-playbook calls, this is usefull when one
want to pass multiple --extra-vars arguments in the
native format giving user a choice to use json, name=value
pairs or path to yaml/json files.

Signed-off-by: Kyr Shatskyy [email protected]

toabctl · 2020-10-29T05:16:07Z

tests/lib/hardware/hardware_base.py

        # Supplied extra_vars from settings always take precedence
        if settings.ANSIBLE_EXTRA_VARS:
-            extra_vars.update(json.loads(settings.ANSIBLE_EXTRA_VARS))


I wonder if we should change the method parameter extra_vars to just expect a string which can either be json or a string starting with @ or even something like version=1.23.45 (see https://docs.ansible.com/ansible/latest/user_guide/playbooks_variables.html#defining-variables-at-runtime fro all possibilities). Then we could just do --extra-vars {extra_vars}

I think we should decouple the method parameter extra_vars, from the settings variable ANSIBLE_EXTRA_VARS.

Because we can apply multiple --extra-vars we should just supply both (--extra-vars {extra_vars} --extra-vars {settings.ANSIBLE_EXTRA_VARS}) giving both the option to be in any format supported by ansible.

I love you are discussing this here, but could you please merge this PR to unblock me, and create another PR to with your suggestion, I am happy you guys want to replicate all the features of ansible in rookcheck, but I don't like you force me to do this in this PR.
My intention in this patch was not to break existing functionality in rookcheck to not interfere someone else work.

Regarding @toabctl comment if you want to just have something like ROOKCHECK_ANSIBLE_EXTRA_VARS=--extra-vars {extra_vars}, it would not go with this, it can be confusing, I would suggest to use ROOKCHECK_ANSIBLE_EXTRA_ARGS for that purpose and we can drop the ROOKCHECK__ANSIBLE_EXTRA_VARS since it is useless.

Regardging @jhesketh comment in the context of rookcheck I don't see any advantage of using multipe --extra-vars when passing the parameters to ansible via tox|pytest, I would prefer to have a single file do not pack every parameter to the command line.

I love you are discussing this here, but could you please merge this PR to unblock me, and create another PR to with your suggestion, I am happy you guys want to replicate all the features of ansible in rookcheck, but I don't like you force me to do this in this PR.

Not looking to re-implement Ansible. My suggestions were for a simplifier and more complete solution.

My intention in this patch was not to break existing functionality in rookcheck to not interfere someone else work.

The suggested changes should not break existing functionality.

Regarding @toabctl comment if you want to just have something like ROOKCHECK_ANSIBLE_EXTRA_VARS=--extra-vars {extra_vars}, it would not go with this, it can be confusing, I would suggest to use ROOKCHECK_ANSIBLE_EXTRA_ARGS for that purpose and we can drop the ROOKCHECK__ANSIBLE_EXTRA_VARS since it is useless.

Regardging @jhesketh comment in the context of rookcheck I don't see any advantage of using multipe --extra-vars when passing the parameters to ansible via tox|pytest, I would prefer to have a single file do not pack every parameter to the command line.

The multiple --extra-vars isn't for having to pack each variable as a parameter. It is because we have extra vars coming from two places. 1) some tests and internals of rookcheck may need to overwrite variables at different times, and 2) the user may want to overwrite variables in configuration.

My suggestion simplifies the implementation by separating the two sources of vars, and passing them in directly giving both the test writer and the configuring user the option to use files, json, or otherwise (as supported by Ansible).

Hope that makes more sense, but let me know if not.

My intention in this patch was not to break existing functionality in rookcheck to not int> > Regarding @toabctl comment if you want to just have something like ROOKCHECK_ANSIBLE_EXTRA_VARS=--extra-vars {extra_vars}, it would not go with this, it can be confusing, I would suggest to use ROOKCHECK_ANSIBLE_EXTRA_ARGS for that purpose and we can drop the ROOKCHECK__ANSIBLE_EXTRA_VARS since it is useless.
Regardging @jhesketh comment in the context of rookcheck I don't see any advantage of using multipe --extra-vars when passing the parameters to ansible via tox|pytest, I would prefer to have a single file do not pack every parameter to the command line.

erfere someone else work.

The suggested changes should not break existing functionality.

To be honest I don't understand the changes you suggest. Looking forward to review your upcoming PR.

The multiple --extra-vars isn't for having to pack each variable as a parameter. It is because we have extra vars coming from two places. 1) some tests and internals of rookcheck may need to overwrite variables at different times, and 2) the user may want to overwrite variables in configuration.

I didn't see that in the existing code, and what the examples say it packs json into the variable. This is why this PR appeared.

My suggestion simplifies the implementation by separating the two sources of vars, and passing them in directly giving both the test writer and the configuring user the option to use files, json, or otherwise (as supported by Ansible).

Hope that makes more sense, but let me know if not.

I understand the sense of original ansible-playbook options very well. What I do not understand is your suggestion, it would be easier if you provided code examples. Anyways we are discussing the code which is not created by me, I was not going to change it, and more the current PR revision does not even touch it.

I hope we merge it asap, so I do not need to cherry-pick it from branch to branch in order to use the functionality.

I think the idea from @jhesketh is #216

@toabctl That looks like reverting his previous change df75e6c

No (unless I'm missing something). My previous change updated the vars by loading in the settings as a python dict and updating them then passing a final version of the dict to Ansible by one --extra-vars. #216 passes the settings as a second --extra-vars when once applied will overwrite anything else in the same order of precedence.

@jhesketh I might be wrong but after Tom updated his initial version of the PR it is not now.

kshtsk · 2020-10-29T11:35:13Z

The jenkins failure is unrelated to the PR.

toabctl · 2020-10-29T14:11:44Z

tests/lib/hardware/hardware_base.py

@@ -159,10 +159,12 @@ def ansible_run_playbook(self, playbook: str,
        else:
            extra_vars_param = ""

+        extra_args = settings.get('ANSIBLE_PLAYBOOK_EXTRA_ARGS', '')


I would prefer to not add another variable that overlaps with ANSIBLE_EXTRA_ARGS

If we go that way, it needs to be documented.

I don't see any serious issues with overlaping, it gives user an option.

It's confusing and everything you can do with ANSIBLE_PLAYBOOK_EXTRA_ARGS can be done already with ANSIBLE_EXTRA_ARGS. So what's the benefit?

There two advantages:

allows multiple usage of --extra-vars, so you can, for example, use more than one json file with properties without prio combining it in one.

allows to provide other options to ansible-playbook rather than --extra-vars, see example in the doc (btw I added it by your request).

This change makes it possible to add extra variables for ansible-playbook calls, this is usefull when one want to pass multiple --extra-vars arguments in the native format giving user a choice to use json, name=value pairs or path to yaml/json files. Signed-off-by: Kyr Shatskyy <[email protected]>

jhesketh · 2020-10-30T06:21:00Z

Does #216 satisfy your needs and supersede this PR?

kshtsk · 2020-10-30T10:30:19Z

Does #216 satisfy your needs and supersede this PR?

Thanks, I think it covers my needs, however I didn't test it, but it does not supersede current version of this PR, because the latter allow more than just passing --extra-vars.

jhesketh · 2020-10-30T12:29:17Z

Does #216 satisfy your needs and supersede this PR?

Thanks, I think it covers my needs, however I didn't test it, but it does not supersede current version of this PR, because the latter allow more than just passing --extra-vars.

It is a little confusing, but it is talking about extra arguments to the ansible-playbook command (eg, pass along "--verbose"). Perhaps extra-params would be less confusing?

kshtsk · 2020-10-30T12:50:43Z

Does #216 satisfy your needs and supersede this PR?

Thanks, I think it covers my needs, however I didn't test it, but it does not supersede current version of this PR, because the latter allow more than just passing --extra-vars.

It is a little confusing, but it is talking about extra arguments to the ansible-playbook command (eg, pass along "--verbose"). Perhaps extra-params would be less confusing?

No, it is not confusing, you are messing vars and args. The variables are internal things in ansible, arguments are the synonym for parameters for a program, same as options.

kshtsk · 2020-10-30T12:53:27Z

Guys, I don't want to spend your time on this anylonger, if you're not okay with it, I close.

jhesketh

I'm fine with this, although I do have hesitations on not having too many settings in general.

@toabctl I'll leave this open for you to respond to the open comments.

I still don't see a benefit to have 2 options. So I would then like to have only this option (ANSIBLE_PLAYBOOK_EXTRA_ARGS) but feel free to merge..

kshtsk requested review from toabctl, jhesketh and brunoleon October 28, 2020 16:15

toabctl reviewed Oct 29, 2020

View reviewed changes

kshtsk force-pushed the wip-ansible-vars branch from f04029f to b0cf0a1 Compare October 29, 2020 10:27

kshtsk changed the title ~~lib/hardware: allow files for ansible extra vars~~ Introduce ANSIBLE_PLAYBOOK_EXTRA_ARGS Oct 29, 2020

kshtsk requested a review from toabctl October 29, 2020 11:35

toabctl previously requested changes Oct 29, 2020

View reviewed changes

kshtsk force-pushed the wip-ansible-vars branch from b0cf0a1 to 0f6e78b Compare October 29, 2020 14:24

jhesketh approved these changes Nov 4, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce ANSIBLE_PLAYBOOK_EXTRA_ARGS #215

Introduce ANSIBLE_PLAYBOOK_EXTRA_ARGS #215

kshtsk commented Oct 28, 2020 •

edited

Loading

toabctl Oct 29, 2020

jhesketh Oct 29, 2020

kshtsk Oct 29, 2020

jhesketh Oct 29, 2020

kshtsk Oct 29, 2020

toabctl Oct 29, 2020

kshtsk Oct 29, 2020

jhesketh Oct 30, 2020

kshtsk Oct 30, 2020

kshtsk commented Oct 29, 2020

toabctl Oct 29, 2020

kshtsk Oct 30, 2020

toabctl Oct 30, 2020

kshtsk Oct 30, 2020

jhesketh commented Oct 30, 2020

kshtsk commented Oct 30, 2020

jhesketh commented Oct 30, 2020

kshtsk commented Oct 30, 2020

kshtsk commented Oct 30, 2020

jhesketh left a comment

Introduce ANSIBLE_PLAYBOOK_EXTRA_ARGS #215

Are you sure you want to change the base?

Introduce ANSIBLE_PLAYBOOK_EXTRA_ARGS #215

Conversation

kshtsk commented Oct 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kshtsk commented Oct 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhesketh commented Oct 30, 2020

kshtsk commented Oct 30, 2020

jhesketh commented Oct 30, 2020

kshtsk commented Oct 30, 2020

kshtsk commented Oct 30, 2020

jhesketh left a comment

Choose a reason for hiding this comment

kshtsk commented Oct 28, 2020 •

edited

Loading