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

Invert condition when validating function name #38533

Conversation

martinwilkerson-scisys
Copy link

SUMMARY

In validate_params the .startswith('arn:') condition for function name validation was reversed, so if the string started arn: the check was for a standard function name i.e. does not contain ':'.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

lambda_policy

ANSIBLE VERSION
ansible 2.5.0
  config file = /etc/ansible/ansible.cfg
  configured module search path = [u'/home/vagrant/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/lib/python2.7/dist-packages/ansible
  executable location = /usr/bin/ansible
  python version = 2.7.12 (default, Dec  4 2017, 14:50:18) [GCC 5.4.0 20160609]
ADDITIONAL INFORMATION

When using the lambda_policy module it was not possible to use an arn for the function_name parameter as the validation checked that strings starting arn: do not contain :.


The `.startswith('arn:')` condition for function name validation was reversed, so if the string started `arn:` the check was for a standard function name i.e. does not contain ':'.
@ansibot
Copy link
Contributor

ansibot commented Apr 10, 2018

cc @mikedlr @pjodouin @willthames
click here for bot help

@ansibot ansibot added aws bug This issue/PR relates to a bug. cloud community_review In order to be merged, this PR must follow the community review workflow. module This issue/PR relates to a module. 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. labels Apr 10, 2018
@webknjaz
Copy link
Member

!component lib/ansible/modules/cloud/amazon/lambda_policy.py

-label needs_triage

@ansibot ansibot removed the needs_triage Needs a first human triage before being processed. label Apr 10, 2018
Copy link

@dumblerod dumblerod left a comment

Choose a reason for hiding this comment

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

I ran into this as well and instead of doing the not, reversed the logic inside the if so the positive is first. Either way works, not sure what is preferable.

@willthames
Copy link
Contributor

Yep, although this is the simplest diff, @JRODJ's solution is preferable as it's more readable.

I have a fix in for that and will update the tests to actually check that they work with a function_arn.

@willthames willthames mentioned this pull request Apr 17, 2018
@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. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Apr 25, 2018
@ryansb
Copy link
Contributor

ryansb commented May 3, 2018

This has been solved via #38863

@ryansb ryansb closed this May 3, 2018
@ansible ansible locked and limited conversation to collaborators May 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
aws bug This issue/PR relates to a bug. cloud module This issue/PR relates to a module. 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. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. support:community This issue/PR relates to code supported by the Ansible community.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants