Process connection:local after processing the magic variables

[dagwieers]

SUMMARY

In a heterogeneous environment where more than one connection-method is being used, one has to set ansible_connection for sets of hosts deliberately. In this case a play/task connection: local stops working as expected because it is superseded by a group/host ansible_connection.

There is no situation where a deliberate play/task connection: local should be replaced with another connection method. (Think Windows and Unix systems mixed)

This fixes #24076.

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

play_context / task_executor

ANSIBLE VERSION

v2.4

[ansibot]

The test ansible-test sanity --test pep8 failed with the following errors:

lib/ansible/playbook/play_context.py:397:1: W293 blank line contains whitespace
test/units/modules/system/test_systemd.py:36:161: E501 line too long (165 > 160 characters)

click here for bot help

[ansibot]

The test ansible-test sanity --test pep8 failed with the following error:

test/units/modules/system/test_systemd.py:36:161: E501 line too long (165 > 160 characters)

click here for bot help

[mattclay]

CI failure in unit tests:

>       self.assertEqual(play_context.connection, 'mock_inventory')
E       AssertionError: 'mocktask' != 'mock_inventory'
E       - mocktask
E       + mock_inventory

test/units/playbook/test_play_context.py:106: AssertionError

[dagwieers]

@mattclay The PEP8 check tells me a line is too long, but it's not something I did. So once more we should not confuse contributors with stuff from other people. Unless you expect people to fix that too in their PR (which will only introduce conflicts if they all do...)

[sivel]

That is resolved in devel, please rebase

needs_rebase

[dagwieers]

@sivel I know the issue itself is resolved, but it shouldn't be exposed to people who do not know what is expected from them in the first place. I am not reporting the PEP8 error, I am reporting the problematic workflow...

[dagwieers]

Can't rebase now BTW as I can only work through the GitHub web-interface now.

[mattclay]

I've removed the broken command and label from this PR. The broken command for the bot is used to stop the bot from further processing on an issue when a bug in the bot is encountered. The bot is currently functioning correctly, so there's no need to disable it.

[mattclay]

No rebase is required for CI to pick up the fixes in devel.

[mattclay]

@dagwieers Determining which test failures are due to a given PR and which are unrelated is a complex problem. The current approach is to comment on compile and sanity test failures, but only label the PR with ci_verified if there is high enough confidence that all CI failures for the PR are due to changes made by the PR.

In the case of the PEP 8 issue above, the comment was made by the bot, but the ci_verified label was not applied. This exposed the cause of the CI failure, while still leaving the PR pending review to determine whether or not there were issues unrelated to the PR which needed to be addressed. There was an unrelated issue, which was corrected and then CI was re-run for the PR.

If you have any suggestions on how to improve this workflow, please let me know, as I'm continuing to refine the workflow to improve CI feedback to users.

[dagwieers]

@mattclay Thanks for the feedback. I was thinking that PEP8 errors on files that are not part of the PR, should not be reported as part of the PR.

Why are we testing any other files than those from the PR for PEP8 ?

[dagwieers]

@jctanner @nitzmahone @bcoca @sivel Your feedback is appreciated :-)

[bcoca]

-1 this reverses precedence and breaks existing workflows, connection is not the same as delegate_to: localhost or local_action. The latter make the changes you want/need as they are a 'full' delegation, while changing connection should not change the other aspects of the host.

[dagwieers]

@bcoca So give me an example use for connection: local.

Because with the current behaviour, connection: local has no real purpose if it suddenly could becomes SSH or WinRM and going to your target node instead.

[bcoca]

I'm fine with connection: local not being a thing, it can be confusing and won't work as people expect, specially with different ansible_python_interpreter

[bcoca]

closing as per public meeting