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

Fix regression introduced in b77abd0491 causing bug in inventory modu… #73429

Merged
merged 2 commits into from Feb 4, 2021

Conversation

msh111
Copy link
Contributor

@msh111 msh111 commented Jan 30, 2021

Fix regression introduced in b77abd0 causing bug in inventory modules which break functionality in user setting use_contrib_script_compatible_sanitization parameter.

Effecting all versions since V2.8.5

SUMMARY

Reverted commit b77abd0

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

amazon.aws.aws_ec2

ADDITIONAL INFORMATION

Inventory module usually have setting "use_contrib_script_compatible_sanitization"
The commit I reverted cause this setting to have no effect


… which break functionality in user setting use_contrib_script_compatible_sanitization parameter.
@ansibot ansibot added affects_2.11 bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. inventory Inventory category needs_triage Needs a first human triage before being processed. new_contributor This PR is the first contribution by a new community member. small_patch support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Jan 30, 2021
@samdoran samdoran removed the needs_triage Needs a first human triage before being processed. label Feb 2, 2021
@sivel
Copy link
Member

sivel commented Feb 2, 2021

Looking at this, I'm not sure what the goal is with reverting this change.

self._sanitize_group_name is an alias of to_safe_group_name which accepts no arguments, but calls:

original_safe(name, force=True, silent=True)

The change this is reverting calls original_safe(name, force=True) where silent defaults to False

So the behavior is the same, only with a change that it will display the warning.

Per the documentation of the [aws_ec2](https://docs.ansible.com/ansible/latest/collections/amazon/aws/aws_ec2_inventory.html) inventory plugin,

For [use_contrib_script_compatible_sanitization] to work you should also turn off the TRANSFORM_INVALID_GROUP_CHARS setting, otherwise the core engine will just use the standard sanitization on top

I must be missing something if you validated this change works.

@sivel
Copy link
Member

sivel commented Feb 2, 2021

Oh, the inventory plugins are being naughty! They are overriding a private method of the base class:

if self.get_option('use_contrib_script_compatible_sanitization'):
self._sanitize_group_name = self._legacy_script_compatible_group_sanitization

See 3e52a6a

@msh111
Copy link
Contributor Author

msh111 commented Feb 3, 2021

Looking at this, I'm not sure what the goal is with reverting this change.

self._sanitize_group_name is an alias of to_safe_group_name which accepts no arguments, but calls:

original_safe(name, force=True, silent=True)

The change this is reverting calls original_safe(name, force=True) where silent defaults to False

So the behavior is the same, only with a change that it will display the warning.

Per the documentation of the [aws_ec2](https://docs.ansible.com/ansible/latest/collections/amazon/aws/aws_ec2_inventory.html) inventory plugin,

For [use_contrib_script_compatible_sanitization] to work you should also turn off the TRANSFORM_INVALID_GROUP_CHARS setting, otherwise the core engine will just use the standard sanitization on top

I must be missing something if you validated this change works.

Yes I validated it works like supposed

Copy link
Contributor

@s-hertel s-hertel left a comment

Choose a reason for hiding this comment

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

Here's some context about why this bit of code was changed and a suggested fix to keep the warning in place #69702 (comment). In 2.8 composed groups became sanitized (silently) and was considered a "regression" - the warning was a compromise, but it accidentally broke allowing plugins to override.
That being said, 2.8 was a while ago and we probably don't need the warning anymore, so this looks fine to me.

@ansibot ansibot added support:community This issue/PR relates to code supported by the Ansible community. and removed small_patch labels Feb 4, 2021
@s-hertel s-hertel merged commit 4315e18 into ansible:devel Feb 4, 2021
@s-hertel
Copy link
Contributor

s-hertel commented Feb 4, 2021

Thanks for the fix. I'm working on adding some documentation for _sanitize_group_name as well.

s-hertel pushed a commit to s-hertel/ansible that referenced this pull request Feb 8, 2021
…nsible#73429)

* Fix regression introduced in b77abd0 causing bug in inventory modules which break functionality in user setting  use_contrib_script_compatible_sanitization  parameter.

* Add changelog

Co-authored-by: s-hertel <19572925+s-hertel@users.noreply.github.com>
(cherry picked from commit 4315e18)
@ansible ansible locked and limited conversation to collaborators Mar 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.11 bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. inventory Inventory category new_contributor This PR is the first contribution by a new community member. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants