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.

Sign up for GitHub

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

Jump to bottom

CmdRunner bugfix #7200

Merged
merged 14 commits into from
Sep 10, 2023
Merged

CmdRunner bugfix #7200

merged 14 commits into from
Sep 10, 2023

Conversation

russoz
Copy link
Collaborator

@russoz russoz commented Sep 3, 2023 •
edited
Loading

SUMMARY

CmdRunner relies on AnsibleModule.get_bin_path() to obtain the full path of the command going to be executed. That method uses the PATH environment variable plus a hard-coded list of default paths. If a command is passed with an absolute path, specially if the path is not in the list of paths known by get_bin_path(), it raises an error and it shouldn't.

ISSUE TYPE
  • Bugfix Pull Request
  • Test Pull Request
COMPONENT NAME

plugins/module_utils/cmd_runner.py
tests/integration/targets/cmd_runner/library/cmd_echo.py
tests/integration/targets/cmd_runner/meta/main.yml
tests/integration/targets/cmd_runner/tasks/test_cmd_echo.yml
tests/integration/targets/cmd_runner/vars/main.yml

@ansibullbot ansibullbot added bug This issue/PR relates to a bug integration tests/integration module_utils module_utils plugins plugin (any type) tests tests labels Sep 3, 2023
@ansibullbot ansibullbot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Sep 3, 2023
@ansibullbot ansibullbot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Sep 3, 2023
@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-7 labels Sep 3, 2023
@ansibullbot ansibullbot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Sep 5, 2023
@russoz @felixfontein
Update plugins/module_utils/cmd_runner.py
eaad1f3 
Co-authored-by: Felix Fontein <felix@fontein.de>
felixfontein
felixfontein reviewed Sep 8, 2023
View reviewed changes
Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Then you also need to adjust the changelog fragment ;-)

changelogs/fragments/7200-cmd-runner-abs-path.yml Outdated Show resolved Hide resolved
@russoz @felixfontein
Update changelogs/fragments/7200-cmd-runner-abs-path.yml
e3f0def 
Co-authored-by: Felix Fontein <felix@fontein.de>
@russoz
Copy link
Collaborator Author

russoz commented Sep 8, 2023

@felixfontein thanks once again for your sharp eyes :-)

@felixfontein felixfontein merged commit 8fa667e into ansible-collections:main Sep 10, 2023
@patchback
Copy link

patchback bot commented Sep 10, 2023 •
edited
Loading

Backport to stable-6: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-6/8fa667eeb7a4e3ceca30794846b27aa289cc0028/pr-7200

Backported as #7227

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

patchback bot pushed a commit that referenced this pull request Sep 10, 2023
@russoz @patchback
CmdRunner bugfix (#7200)
5e1e36b 
* cmd_runner module utils: fix bug when passing absolute path not in standard search paths

* improved tests

* changed /usr/bin/echo to /bin/echo for the sake of alpine

* fixed error messaging for last testcase

* add condition to test cases, and remove macos from troubling ones

* fix templating

* fix templating

* exclude centos 6 from testcases copying echo to tmp dir

* try different way of specifying version

* trying trick for old jinjas

* use os.path.isabs() to determine if path is absolute

* add changelog frag

* Update plugins/module_utils/cmd_runner.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update changelogs/fragments/7200-cmd-runner-abs-path.yml

Co-authored-by: Felix Fontein <felix@fontein.de>

---------

Co-authored-by: Felix Fontein <felix@fontein.de>
(cherry picked from commit 8fa667e)
@patchback patchback bot mentioned this pull request Sep 10, 2023
Merged
@patchback
Copy link

patchback bot commented Sep 10, 2023 •
edited
Loading

Backport to stable-7: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-7/8fa667eeb7a4e3ceca30794846b27aa289cc0028/pr-7200

Backported as #7228

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

@felixfontein felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Sep 10, 2023
patchback bot pushed a commit that referenced this pull request Sep 10, 2023
@russoz @patchback
CmdRunner bugfix (#7200)
1ac0da3 
* cmd_runner module utils: fix bug when passing absolute path not in standard search paths

* improved tests

* changed /usr/bin/echo to /bin/echo for the sake of alpine

* fixed error messaging for last testcase

* add condition to test cases, and remove macos from troubling ones

* fix templating

* fix templating

* exclude centos 6 from testcases copying echo to tmp dir

* try different way of specifying version

* trying trick for old jinjas

* use os.path.isabs() to determine if path is absolute

* add changelog frag

* Update plugins/module_utils/cmd_runner.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update changelogs/fragments/7200-cmd-runner-abs-path.yml

Co-authored-by: Felix Fontein <felix@fontein.de>

---------

Co-authored-by: Felix Fontein <felix@fontein.de>
(cherry picked from commit 8fa667e)
@patchback patchback bot mentioned this pull request Sep 10, 2023
Merged
@felixfontein
Copy link
Collaborator

@russoz thanks for fixing this!

felixfontein pushed a commit that referenced this pull request Sep 10, 2023
@patchback @russoz
[PR #7200/8fa667ee backport][stable-7] CmdRunner bugfix (#7228)
c94fa61 
CmdRunner bugfix (#7200)

* cmd_runner module utils: fix bug when passing absolute path not in standard search paths

* improved tests

* changed /usr/bin/echo to /bin/echo for the sake of alpine

* fixed error messaging for last testcase

* add condition to test cases, and remove macos from troubling ones

* fix templating

* fix templating

* exclude centos 6 from testcases copying echo to tmp dir

* try different way of specifying version

* trying trick for old jinjas

* use os.path.isabs() to determine if path is absolute

* add changelog frag

* Update plugins/module_utils/cmd_runner.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update changelogs/fragments/7200-cmd-runner-abs-path.yml

Co-authored-by: Felix Fontein <felix@fontein.de>

---------

Co-authored-by: Felix Fontein <felix@fontein.de>
(cherry picked from commit 8fa667e)

Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com>
felixfontein pushed a commit that referenced this pull request Sep 10, 2023
@patchback @russoz
[PR #7200/8fa667ee backport][stable-6] CmdRunner bugfix (#7227)
dc7aba3 
CmdRunner bugfix (#7200)

* cmd_runner module utils: fix bug when passing absolute path not in standard search paths

* improved tests

* changed /usr/bin/echo to /bin/echo for the sake of alpine

* fixed error messaging for last testcase

* add condition to test cases, and remove macos from troubling ones

* fix templating

* fix templating

* exclude centos 6 from testcases copying echo to tmp dir

* try different way of specifying version

* trying trick for old jinjas

* use os.path.isabs() to determine if path is absolute

* add changelog frag

* Update plugins/module_utils/cmd_runner.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update changelogs/fragments/7200-cmd-runner-abs-path.yml

Co-authored-by: Felix Fontein <felix@fontein.de>

---------

Co-authored-by: Felix Fontein <felix@fontein.de>
(cherry picked from commit 8fa667e)

Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com>
@russoz russoz deleted the cmd-runner-bugfix branch September 10, 2023 08:15
etrombly pushed a commit to etrombly/community.general that referenced this pull request Oct 25, 2023
@russoz @felixfontein
CmdRunner bugfix (ansible-collections#7200)
4a472af 
* cmd_runner module utils: fix bug when passing absolute path not in standard search paths

* improved tests

* changed /usr/bin/echo to /bin/echo for the sake of alpine

* fixed error messaging for last testcase

* add condition to test cases, and remove macos from troubling ones

* fix templating

* fix templating

* exclude centos 6 from testcases copying echo to tmp dir

* try different way of specifying version

* trying trick for old jinjas

* use os.path.isabs() to determine if path is absolute

* add changelog frag

* Update plugins/module_utils/cmd_runner.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update changelogs/fragments/7200-cmd-runner-abs-path.yml

Co-authored-by: Felix Fontein <felix@fontein.de>

---------

Co-authored-by: Felix Fontein <felix@fontein.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felixfontein felixfontein felixfontein approved these changes

Assignees
No one assigned
Labels
bug This issue/PR relates to a bug integration tests/integration module_utils module_utils plugins plugin (any type) tests tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@russoz @felixfontein @ansibullbot