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 ec2_ami block_device_mapping volume_size to be int in 2.5 #40938

Merged
merged 2 commits into from
Jun 4, 2018

Conversation

mentos1386
Copy link
Contributor

SUMMARY

boto3 added type validation to many methods #28506. Similar to #18999 and #32291, the ec2_ami module didn't cast the volume_size parameter to int before passing it to boto. This was fine if the yaml used an integer value, but if a string was used (because of templating, or variable substitution mostly), a type error was raised.

This #32738 PR will make this PR obsolete. But until then, i think this is a valid fix.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

ec2_ami

ANSIBLE VERSION
ansible 2.5.3
  config file = None
  configured module search path = ['/root/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /app/vendor/python3/lib/python3.5/site-packages/ansible
  executable location = /app/vendor/python3/bin/ansible
  python version = 3.5.3 (default, Jan 19 2017, 14:11:04) [GCC 6.3.0 20170118]
ADDITIONAL INFORMATION

If we have a task like this:

- ec2_ami:
    name: "{{ ami_name }}"
    state: present
    device_mapping:
      - device_name: /dev/sda1
        volume_size: "{{ ami_size|int }}"
# ...

We will get an error that volume_size isn't correct type:

TASK [ec2_ami] *****************************************************************
An exception occurred during task execution. To see the full traceback, use -vvv. The error was: Invalid type for parameter BlockDeviceMappings[0].Ebs.VolumeSize, value: 10, type: <class 'str'>, valid types: <class 'int'>

After applying the PR, there is no issues anymore.

@ansibot
Copy link
Contributor

ansibot commented May 31, 2018

@ansibot ansibot added affects_2.7 This issue/PR affects Ansible v2.7 aws bug This issue/PR relates to a bug. cloud committer_review In order to be merged, this PR must follow the certified 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. python3 support:certified This issue/PR relates to certified code. traceback This issue/PR includes a traceback. labels May 31, 2018
@mentos1386 mentos1386 changed the title Fix ec2_ami block_device_mapping volume_size to be int Fix ec2_ami block_device_mapping volume_size to be int in 2.5 Jun 1, 2018
@mkrizek mkrizek removed the needs_triage Needs a first human triage before being processed. label Jun 1, 2018
new_item = dict_object.get(attribute)
if new_item is not None:
value = dict_object.get(attribute)
if type is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really like overriding the type builtin - can we use attribute_type as parameter or similar?

new_item = dict_object.get(attribute)
if new_item is not None:
value = dict_object.get(attribute)
Copy link
Contributor

Choose a reason for hiding this comment

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

You're setting value exactly the same way that new_item was set. Either reuse new_item or use value instead of new_item

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed committer_review In order to be merged, this PR must follow the certified review workflow. labels Jun 4, 2018
renamed `type` to `attribute_type`
reused `new_item` instead of creating new variable `value`
@mentos1386
Copy link
Contributor Author

@willthames Fixed CR issues

@ansibot ansibot added committer_review In order to be merged, this PR must follow the certified review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jun 4, 2018
@willthames
Copy link
Contributor

LGTM now, thanks for the fixes - I've asked @s-hertel and @ryansb for a second opinion

@ryansb ryansb merged commit ab96a84 into ansible:devel Jun 4, 2018
@mentos1386
Copy link
Contributor Author

@willthames is there a way for me to be informed when this is released? Also, is this going to be released for 2.5 as well?

@willthames
Copy link
Contributor

@mentos1386 not that I know of - it'll go out with 2.7 but it's definitely a candidate for backporting to 2.5 and 2.6

To use it prior to release see: http://willthames.github.io/2017/12/12/using-updated-modules-with-stable-ansible.html

willthames pushed a commit to willthames/ansible that referenced this pull request Jun 6, 2018
…e#40938)

* fix ec2_ami block_device_mapping size to be int

* fixed cr issues

renamed `type` to `attribute_type`
reused `new_item` instead of creating new variable `value`

(cherry picked from commit ab96a84)
willthames pushed a commit to willthames/ansible that referenced this pull request Jun 6, 2018
…e#40938)

* fix ec2_ami block_device_mapping size to be int

* fixed cr issues

renamed `type` to `attribute_type`
reused `new_item` instead of creating new variable `value`

(cherry picked from commit ab96a84)
@willthames
Copy link
Contributor

Backport PRs created for 2.5 and 2.6

mattclay pushed a commit that referenced this pull request Jun 7, 2018
#41216)

* Fix ec2_ami block_device_mapping volume_size to be int in 2.5 (#40938)

* fix ec2_ami block_device_mapping size to be int

* fixed cr issues

renamed `type` to `attribute_type`
reused `new_item` instead of creating new variable `value`

(cherry picked from commit ab96a84)

* changelog
nitzmahone pushed a commit that referenced this pull request Jun 8, 2018
#41217)

* Fix ec2_ami block_device_mapping volume_size to be int in 2.5 (#40938)

* fix ec2_ami block_device_mapping size to be int

* fixed cr issues

renamed `type` to `attribute_type`
reused `new_item` instead of creating new variable `value`

(cherry picked from commit ab96a84)

* changelog

(cherry picked from commit e6cd727)

* changelog format tweak
jacum pushed a commit to jacum/ansible that referenced this pull request Jun 26, 2018
…e#40938)

* fix ec2_ami block_device_mapping size to be int

* fixed cr issues

renamed `type` to `attribute_type`
reused `new_item` instead of creating new variable `value`
@ansible ansible locked and limited conversation to collaborators Jun 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.7 This issue/PR affects Ansible v2.7 aws bug This issue/PR relates to a bug. cloud committer_review In order to be merged, this PR must follow the certified review workflow. module This issue/PR relates to a module. new_contributor This PR is the first contribution by a new community member. python3 support:certified This issue/PR relates to certified code. traceback This issue/PR includes a traceback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants