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

command: Remove executable parameter when using command #23062

Merged
merged 1 commit into from
Jul 11, 2017

Conversation

dagwieers
Copy link
Contributor

@dagwieers dagwieers commented Mar 29, 2017

SUMMARY

This was discussed with the core team and removing this option was preferred.
For backward compatibility we accept the parameter, but warn the user instead.

This PR also includes a change related to warnings, we now use the warning-infrastructure, this avoids empty warnings-dictionary in the output if there's no warning.

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

command

ANSIBLE VERSION

v2.4

@dagwieers
Copy link
Contributor Author

@nitzmahone @jimi-c @bcoca Is this implementation acceptable ?

@ansibot ansibot added affects_2.4 This issue/PR affects Ansible v2.4 bugfix_pull_request core_review In order to be merged, this PR must follow the core review workflow. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. labels Mar 29, 2017
Copy link
Member

@nitzmahone nitzmahone left a comment

Choose a reason for hiding this comment

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

Looks good to me- I don't think returning an empty warnings collection is a problem, but others might disagree.

@nitzmahone nitzmahone requested a review from bcoca March 29, 2017 00:48
@Qalthos Qalthos removed the needs_triage Needs a first human triage before being processed. label Mar 30, 2017
@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 Apr 11, 2017
@dagwieers
Copy link
Contributor Author

dagwieers commented Apr 18, 2017

@nitzmahone The empty warnings was not a concern to me, I simply wanted to make use of the new module.warn() infrastructure. More examples of it in use may help people do the right thing :-)

@bcoca So what needs to happen to get this going ?

@ansibot ansibot added 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. support:core This issue/PR relates to code supported by the Ansible Engineering Team. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Jun 23, 2017
@ansibot ansibot removed module This issue/PR relates to a module. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html 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 Jul 11, 2017
@@ -157,6 +141,10 @@ def main():
removes = module.params['removes']
warn = module.params['warn']

if not shell and executable:
module.warn('The parameter "executable" is not supported with the "command" module. Not using "%s".' % executable)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add the version executable went away:

module.warn('As of Ansible 2.4, the parameter "executable" is no longer supported with the "command" module. Not using "%s".' % executable) 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed. Thanks !

@dagwieers dagwieers force-pushed the win_command-executable branch 2 times, most recently from 00cecef to 5094b33 Compare July 11, 2017 20:03
This was discussed with the core team and removing this option was preferred.
For backward compatibility we accept the parameter, but warn the user instead.
@ansibot ansibot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. labels Jul 11, 2017
@abadger abadger merged commit 8a1ad92 into ansible:devel Jul 11, 2017
@abadger
Copy link
Contributor

abadger commented Jul 11, 2017

merged to devel. Thanks dag!

@ansibot ansibot added the bug This issue/PR relates to a bug. label Mar 6, 2018
@ansible ansible locked and limited conversation to collaborators Apr 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.4 This issue/PR affects Ansible v2.4 bug This issue/PR relates to a bug. 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