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

do not add colliding aliases to argument_spec #62315 #71557

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

Conversation

terencehonles
Copy link

@terencehonles terencehonles commented Sep 1, 2020

SUMMARY

This is a refreshed PR based on #63017. While #62315 is already fixed on devel and I had opened #71555 to just incorporate the test in test/integration/targets/blockinfile/tasks/main.yml (which is also included in this PR) I realized this was still a bug to have the FILE_COMMON_ARGUMENTS stomping on module aliases, and I opened this PR to address that.

As mentioned in #71555 the issue #62315 was fixed with #66389, and that will go out with the next stable release.

While I have already closed #71555, I do want to call out my comment on that PR, that figuring out how to write a test for Ansible was rather difficult and I struggled with trying to even start the test runner. Eventually I stumbled upon the --venv option, and I really think that should be much more front and center because it is very likely the option you want to be running with (that or possibly --docker). As an initial contributor I don't want to worry about how the packages are installed and I don't want the test system to install packages globally on my system. I'm calling this out because I'm an advanced Python user and I am also very stubborn and wanted to get this fix in. I don't know if this is something you can count on from your average developer. Just my two cents, and I mean what I'm saying as constructively as possible.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

module_utils (all modules)

@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. files Files category needs_triage Needs a first human triage before being processed. 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. test This PR relates to tests. labels Sep 1, 2020
@ansibot
Copy link
Contributor

ansibot commented Sep 1, 2020

The test ansible-test sanity --test pylint [explain] failed with 1 error:

test/lib/ansible_test/_data/sanity/validate-modules/validate_modules/module_args.py:157:25: blacklisted-name: Black listed name "_"

click here for bot help

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Sep 1, 2020
@Akasurde Akasurde requested a review from sivel Sep 2, 2020
@sivel sivel requested review from samdoran and removed request for sivel Sep 2, 2020
@terencehonles terencehonles force-pushed the fix-FILE_COMMON_ARGUMENTS-alias-collisions branch from fe2f954 to a5369a1 Compare Sep 2, 2020
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. core_review In order to be merged, this PR must follow the core review workflow. labels Sep 2, 2020
@samdoran samdoran added the ci_verified Changes made in this PR are causing tests to fail. label Sep 2, 2020
@terencehonles
Copy link
Author

terencehonles commented Sep 2, 2020

Looking at the tests it looks like moving the FILE_COMMON_ARGUMENTS registration to after the aliases are resolved breaks the aliases coming from FILE_COMMON_ARGUMENTS. I'll see about updating that when I can.

@Shrews Shrews removed the needs_triage Needs a first human triage before being processed. label Sep 3, 2020
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Sep 11, 2020
Based on the research by Matt Martz [1]_, aliases such as "content" in
"blockinfile" would be added to the argument_spec. For ansible#62315, before
the change in f725dce, this would mean that "content" would be
added with "no_log=True". As a result, the the output was redacted from
the diff. While this change is no longer needed to make sure "content"
is not added with "no_log=True", it is probably still an error to not be
checking for alias collisions when adding the FILE_COMMON_ARGUMENTS to
the argument_spec.

.. [1] ansible#62315 (comment)
@terencehonles terencehonles force-pushed the fix-FILE_COMMON_ARGUMENTS-alias-collisions branch from a5369a1 to 9a5d7c9 Compare Oct 7, 2020
@terencehonles
Copy link
Author

terencehonles commented Oct 7, 2020

I think this may fix the issue exposed by the tests, but I'm just going to let the CI build this since I didn't uncover this issue when I had tested locally.

@ansibot ansibot removed ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Oct 7, 2020
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. core_review In order to be merged, this PR must follow the core review workflow. labels Oct 7, 2020
@samdoran samdoran added the ci_verified Changes made in this PR are causing tests to fail. label Oct 9, 2020
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Oct 17, 2020
@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Nov 2, 2020
@ansibot ansibot added pre_azp This PR was last tested before migration to Azure Pipelines. and removed ci_verified Changes made in this PR are causing tests to fail. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Dec 4, 2020
@samdoran samdoran added the arg_spec Related to argument spec parsing or validation label Feb 9, 2021
if (k not in self.argument_spec
and k not in self.aliases
and not any(
a in self.argument_spec or a in self.aliases
for a in file_common_aliases)):
Copy link
Contributor

@s-hertel s-hertel Aug 17, 2022

Choose a reason for hiding this comment

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

This would be more readable by setting some intermediate variables.

@s-hertel s-hertel self-assigned this Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects_2.11 arg_spec Related to argument spec parsing or validation bug This issue/PR relates to a bug. files Files category needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. new_contributor This PR is the first contribution by a new community member. pre_azp This PR was last tested before migration to Azure Pipelines. 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. test This PR relates to tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants