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 parameter validation in ecs_task #402

Merged

Conversation

mjmayer
Copy link
Contributor

@mjmayer mjmayer commented Feb 9, 2021

SUMMARY

Fixes incorrect logic in parameter validation.

if 'task_definition' not in module.params and module.params['task_definition'] is None:
    module.fail_json(msg="To stop a task, a task definition must be specified")

This will not evaluate to true even if task_definition not provided because module.params['task_definiton'] = None by default. Which means the condition 'task_definition' not in module.params is always false.

Fixes #401

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

ecs_task

@ansibullbot
Copy link

@ansibullbot ansibullbot added bug This issue/PR relates to a bug community_review module module needs_triage new_contributor Help guide this first time contributor plugins plugin (any type) labels Feb 9, 2021
Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

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

Thanks for this PR.

I think it would be good to add a couple of entries to the operation description (line 21)

- When I(operation=start) both I(task_definition) and I(container_instances) must be set.
- When I(operation=stop) both I(task_definition) and I(task) must be set.

Integration tests would also be a nice-to-have, but since they seem to be completely missing right now it's totally reasonable for you not to add them.

plugins/modules/ecs_task.py Outdated Show resolved Hide resolved
@ansibullbot ansibullbot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed community_review labels Feb 10, 2021
@mjmayer mjmayer force-pushed the fix_task_parameter_validation branch from 0fbf051 to 8f3f5c6 Compare February 10, 2021 18:52
@mjmayer mjmayer force-pushed the fix_task_parameter_validation branch from 8f3f5c6 to ed89482 Compare February 11, 2021 00:28
@ansibullbot ansibullbot added community_review 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 community_review labels Feb 11, 2021
Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

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

As a future thing, I find using a temporary variable (same as we do with argument_spec) preferable to the way you split required_if to stay within 80 chars. In this case it would have allowed you to group things a little more clearly. But that's a personal style thing.

Thanks for fixing this.

Copy link
Member

@markuman markuman left a comment

Choose a reason for hiding this comment

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

LGTM

@ansibullbot ansibullbot added community_review and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Feb 11, 2021
@tremble tremble merged commit c786227 into ansible-collections:main Feb 11, 2021
@tremble
Copy link
Contributor

tremble commented Feb 11, 2021

Thanks for taking the time to submit this PR. The fix should be available in version 1.4.0 which we expect to release in the near future.

ethemcemozkan pushed a commit to ethemcemozkan/community.aws that referenced this pull request Feb 18, 2021
* Fix parameter validation in ecs_task
* Require cluster parameter in ecs_task module
* Move parameter validation to AnsibleAWSModule
* Fix pep8 formatting line too long
* changelog
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request Jul 19, 2021
* Fix parameter validation in ecs_task
* Require cluster parameter in ecs_task module
* Move parameter validation to AnsibleAWSModule
* Fix pep8 formatting line too long
* changelog
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request Jul 19, 2021
* Fix parameter validation in ecs_task
* Require cluster parameter in ecs_task module
* Move parameter validation to AnsibleAWSModule
* Fix pep8 formatting line too long
* changelog
danielcotton pushed a commit to danielcotton/community.aws that referenced this pull request Nov 23, 2021
* Fix parameter validation in ecs_task
* Require cluster parameter in ecs_task module
* Move parameter validation to AnsibleAWSModule
* Fix pep8 formatting line too long
* changelog
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 community_review module module needs_triage new_contributor Help guide this first time contributor plugins plugin (any type)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parameter validation logic in ecs_task does not work as intended
4 participants