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

dig: Fix evaluation of boolean parameters #5129

Merged
merged 5 commits into from Aug 20, 2022

Conversation

mu1f407
Copy link
Contributor

@mu1f407 mu1f407 commented Aug 20, 2022

SUMMARY

Fixes evaluation of falsy boolean parameters in dig lookup plugin.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

community.general.dig lookup plugin

ADDITIONAL INFORMATION

Plugin currently uses bool() function to convert string boolean values to boolean type. This function in not the right one for this task as is returns True for every non-empty string:

Python 3.9.7 (default, Sep 13 2021, 08:18:39) 
[GCC 8.5.0 20210514 (Red Hat 8.5.0-3)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> bool('False')
True
>>> bool('no')
True

There is function in ansible.module_utils for this which correctly handles string-to-boolean conversion.

I also added tests for dig lookup and this issue specifically.

@ansibullbot
Copy link
Collaborator

cc @jpmens
click here for bot help

@ansibullbot ansibullbot added bug This issue/PR relates to a bug integration tests/integration lookup lookup plugin new_contributor Help guide this first time contributor plugins plugin (any type) tests tests labels Aug 20, 2022
@jpmens
Copy link
Contributor

jpmens commented Aug 20, 2022

Thank you, @mu1f407

@jpmens
Copy link
Contributor

jpmens commented Aug 20, 2022

shipit

@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-4 labels Aug 20, 2022
@mu1f407
Copy link
Contributor Author

mu1f407 commented Aug 20, 2022

Done. Thanks for hints @felixfontein.

@ansibullbot
Copy link
Collaborator

The test licenses failed with 2 errors:

tests/integration/targets/lookup_dig/meta/main.yml:0:0: found no copyright notice
tests/integration/targets/lookup_dig/meta/main.yml:0:0: must have at least one license

click here for bot help

@ansibullbot ansibullbot added ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Aug 20, 2022
@github-actions
Copy link

github-actions bot commented Aug 20, 2022

Docs Build 📝

Thank you for contribution!✨

This PR has been merged and your docs changes will be incorporated when they are next published.

@felixfontein felixfontein merged commit 3c2d7eb into ansible-collections:main Aug 20, 2022
@patchback
Copy link

patchback bot commented Aug 20, 2022

Backport to stable-4: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply 3c2d7eb on top of patchback/backports/stable-4/3c2d7eb193ac6676a054396c2d41328f23862104/pr-5129

Backporting merged PR #5129 into main

  1. Ensure you have a local repo clone of your fork. Unless you cloned it
    from the upstream, this would be your origin remote.
  2. Make sure you have an upstream repo added as a remote too. In these
    instructions you'll refer to it by the name upstream. If you don't
    have it, here's how you can add it:
    $ git remote add upstream https://github.com/ansible-collections/community.general.git
  3. Ensure you have the latest copy of upstream and prepare a branch
    that will hold the backported code:
    $ git fetch upstream
    $ git checkout -b patchback/backports/stable-4/3c2d7eb193ac6676a054396c2d41328f23862104/pr-5129 upstream/stable-4
  4. Now, cherry-pick PR dig: Fix evaluation of boolean parameters #5129 contents into that branch:
    $ git cherry-pick -x 3c2d7eb193ac6676a054396c2d41328f23862104
    If it'll yell at you with something like fatal: Commit 3c2d7eb193ac6676a054396c2d41328f23862104 is a merge but no -m option was given., add -m 1 as follows intead:
    $ git cherry-pick -m1 -x 3c2d7eb193ac6676a054396c2d41328f23862104
  5. At this point, you'll probably encounter some merge conflicts. You must
    resolve them in to preserve the patch from PR dig: Fix evaluation of boolean parameters #5129 as close to the
    original as possible.
  6. Push this branch to your fork on GitHub:
    $ git push origin patchback/backports/stable-4/3c2d7eb193ac6676a054396c2d41328f23862104/pr-5129
  7. Create a PR, ensure that the CI is green. If it's not — update it so that
    the tests and any other checks pass. This is it!
    Now relax and wait for the maintainers to process your pull request
    when they have some cycles to do reviews. Don't worry — they'll tell you if
    any improvements are necessary when the time comes!

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

@patchback
Copy link

patchback bot commented Aug 20, 2022

Backport to stable-5: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-5/3c2d7eb193ac6676a054396c2d41328f23862104/pr-5129

Backported as #5137

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

@felixfontein
Copy link
Collaborator

@mu1f407 thanks for fixing this!
@jpmens thanks for reviewing!

patchback bot pushed a commit that referenced this pull request Aug 20, 2022
* Add lookup_dig tests

* Fix boolean evaluation

* Add changelog fragment

* Apply review changes

* Add license

(cherry picked from commit 3c2d7eb)
This was referenced Nov 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue/PR relates to a bug ci_verified Push fixes to PR branch to re-run CI integration tests/integration lookup lookup plugin needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR new_contributor Help guide this first time contributor plugins plugin (any type) tests tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants