ecs_taskdefinition: ensure cast to integer and idempotency fix #574

alinabuzachis · 2021-05-10T10:59:03Z

SUMMARY

ecs_taskdefinition: Ensure cast to integer and fix taskdefinition creation idempotency.
Fixes: #570

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

ecs_taskdefinition

amackenz2048 · 2021-05-11T21:26:04Z

I get an error with this branch when using maxSwap and swappiness:

File \"/tmp/ansible_ecs_taskdefinition_payload_tdgmg8iq/ansible_ecs_taskdefinition_payload.zip/ansible/modules/ecs_taskdefinition.py\", line 832, in main\nTypeError: '<' not supported between instances of 'str' and 'int'\n",

Adding some casts fixes it. Not sure if it would be better to do inline or ensure that container has the ints.

diff --git a/plugins/modules/ecs_taskdefinition.py b/plugins/modules/ecs_taskdefinition.py
index 577b0ed..c6e488b 100644
--- a/plugins/modules/ecs_taskdefinition.py
+++ b/plugins/modules/ecs_taskdefinition.py
@@ -829,12 +829,12 @@ def main():
 
                 if linux_param == 'maxSwap' and launch_type == 'FARGATE':
                     module.fail_json(msg='maxSwap parameter is not supported with the FARGATE launch type.')
-                elif linux_param == 'maxSwap' and container['linuxParameters']['maxSwap'] < 0:
+                elif linux_param == 'maxSwap' and int(container['linuxParameters']['maxSwap']) < 0:
                     module.fail_json(msg='Accepted values for maxSwap are 0 or any positive integer.')
 
                 if (
                     linux_param == 'swappiness' and
-                    (container['linuxParameters']['swappiness'] < 0 or container['linuxParameters']['swappiness'] > 100)
+                    (int(container['linuxParameters']['swappiness']) < 0 or int(container['linuxParameters']['swappiness']) > 100)
                 ):
                     module.fail_json(msg='Accepted values for swappiness are whole numbers between 0 and 100.')

alinabuzachis · 2021-05-11T21:53:21Z

I get an error with this branch when using maxSwap and swappiness:

File \"/tmp/ansible_ecs_taskdefinition_payload_tdgmg8iq/ansible_ecs_taskdefinition_payload.zip/ansible/modules/ecs_taskdefinition.py\", line 832, in main\nTypeError: '<' not supported between instances of 'str' and 'int'\n",

Adding some casts fixes it. Not sure if it would be better to do inline or ensure that container has the ints.

diff --git a/plugins/modules/ecs_taskdefinition.py b/plugins/modules/ecs_taskdefinition.py
index 577b0ed..c6e488b 100644
--- a/plugins/modules/ecs_taskdefinition.py
+++ b/plugins/modules/ecs_taskdefinition.py
@@ -829,12 +829,12 @@ def main():
 
                 if linux_param == 'maxSwap' and launch_type == 'FARGATE':
                     module.fail_json(msg='maxSwap parameter is not supported with the FARGATE launch type.')
-                elif linux_param == 'maxSwap' and container['linuxParameters']['maxSwap'] < 0:
+                elif linux_param == 'maxSwap' and int(container['linuxParameters']['maxSwap']) < 0:
                     module.fail_json(msg='Accepted values for maxSwap are 0 or any positive integer.')
 
                 if (
                     linux_param == 'swappiness' and
-                    (container['linuxParameters']['swappiness'] < 0 or container['linuxParameters']['swappiness'] > 100)
+                    (int(container['linuxParameters']['swappiness']) < 0 or int(container['linuxParameters']['swappiness']) > 100)
                 ):
                     module.fail_json(msg='Accepted values for swappiness are whole numbers between 0 and 100.')

Oh, that's true. It's missing the cast to int there. I fix it right now. Thank you for trying it and letting me know. Can you try again please? Thanks.

amackenz2048 · 2021-05-11T21:57:24Z

That works for me!

markuman

Some nits, otherwise looks good to me.

markuman · 2021-05-19T11:04:58Z

plugins/modules/ecs_taskdefinition.py

+                    for param in ('maxSwap', 'swappiness', 'sharedMemorySize'):
+                        if param in linux_param:
+                            container['linuxParameters'][param] = int(container['linuxParameters'][param])


Suggested change

for param in ('maxSwap', 'swappiness', 'sharedMemorySize'):

if param in linux_param:

container['linuxParameters'][param] = int(container['linuxParameters'][param])

if linux_param in ('maxSwap', 'swappiness', 'sharedMemorySize'):`

container['linuxParameters'][param] = int(container['linuxParameters'][param])

@markuman Thank you for reviewing. I used the for loop here because I need to cast to int all these parameters ('maxSwap', 'swappiness', 'sharedMemorySize'). The user can use all together.

markuman · 2021-05-19T11:06:10Z

plugins/modules/ecs_taskdefinition.py

+                for limits_mapping in container['ulimits']:
+                    for limit in ('softLimit', 'hardLimit'):
+                        if limit in limits_mapping:
+                            limits_mapping[limit] = int(limits_mapping[limit])


Suggested change

for limits_mapping in container['ulimits']:

for limit in ('softLimit', 'hardLimit'):

if limit in limits_mapping:

limits_mapping[limit] = int(limits_mapping[limit])

for limits_mapping in container['ulimits']:

for limit in ('softLimit', 'hardLimit'):

if limit == limits_mapping:

limits_mapping[limit] = int(limits_mapping[limit])

@markuman limits_mapping is a dict, for this reason I used if limit in limits_mapping.

markuman · 2021-05-19T11:07:55Z

plugins/modules/ecs_taskdefinition.py

                    module.fail_json(msg='maxSwap parameter is not supported with the FARGATE launch type.')
-                elif linux_param.get('maxSwap') and linux_param['maxSwap'] < 0:
+                elif linux_param == 'maxSwap' and int(container['linuxParameters']['maxSwap']) < 0:


What happen if key maxSwap does not exist? Is it possible? I've no idea.

same for swappiness

@markuman maxSwap and swappiness are optional and they can be specified within linuxParameters. If this is what you meant. This flow here is only executed when the parameters are specified.

markuman

LGTM

markuman · 2021-07-07T18:51:03Z

This shoud be merged before next release. Because it fixes more than the known issue.
With the current release (1.5.0) the entire linuxParameter.capabilities is not useable (worked with 2.9.12).

        containers:
          - name: somename
            linuxParameters:
              capabilities:
                add:
                  - NET_BIND_SERVICE

Currently it results in

line 806, in main\nAttributeError: 'str' object has no attribute 'get'\n"

This PR fixes also this case.

Maybe the integrationtest should also include something like

ecs_task_containers_with_linux_parameters:
- name: "{{ ecs_task_name }}"
  image: "{{ ecs_task_image_path }}"
  essential: true
  memory: "{{ ecs_task_memory }}"
  linuxParameters:
    capabilities:
      add:
        - NET_BIND_SERVICE
  portMappings:
  - containerPort: "{{ ecs_task_container_port }}"
    hostPort: "{{ ecs_task_host_port|default(0) }}"
  mountPoints: "{{ ecs_task_mount_points|default([]) }}"

fyi @alinabuzachis @jillr

ansibullbot · 2021-07-07T18:52:08Z

cc @Java1Guy @s-hertel @tremble @wimnat
click here for bot help

tremble · 2021-07-07T19:25:55Z

plugins/modules/ecs_taskdefinition.py

@@ -190,7 +192,7 @@
            linuxParameters:
                description: Linux-specific modifications that are applied to the container, such as Linux kernel capabilities.
                required: False
-                type: list
+                type: dict


This looks like it's going from a list of dicts to a dict, is this not a breaking change? (Although I guess we can do this with 2.0.0

(Imho) it never worked as a list. I guess it was one of the transformation errors when elements where introduced.

jillr

@alinabuzachis This lgtm, could you please add a changelog? As tremble noted we should call out that linuxParameters requires a dict (and that list was a bug that never worked)

Signed-off-by: Alina Buzachis <[email protected]>

markuman

LGTM

ansible-zuul

LGTM!

) [3.0.0] Bump minimal botocore version to 1.19.0 SUMMARY In preparation for release 3.0.0, bump the minimal botocore version ISSUE TYPE Feature Pull Request COMPONENT NAME README.md plugins/doc_fragments/aws.py plugins/lookup/aws_account_attribute.py plugins/lookup/aws_secret.py plugins/lookup/aws_ssm.py plugins/module_utils/core.py plugins/modules/s3_bucket.py requirements.txt tests/integration/constraints.txt tests/integration/targets/setup_botocore_pip/defaults/main.yml tests/unit/constraints.txt tests/unit/module_utils/core/ansible_aws_module/test_minimal_versions.py ADDITIONAL INFORMATION botocore $ git show 1.19.0 tag 1.19.0 Tagger: aws-sdk-python-automation <[email protected]> Date: Mon Oct 19 18:09:03 2020 +0000 Tagging 1.19.0 release. boto3 $ git show 1.16.0 tag 1.16.0 Tagger: aws-sdk-python-automation <[email protected]> Date: Mon Oct 19 18:08:56 2020 +0000 Tagging 1.16.0 release. Reviewed-by: Alina Buzachis <None> Reviewed-by: None <None>

alinabuzachis changed the title ~~ecs_taskdefinition: ensure cast to integers and fix idempotency of taskdefinition creation~~ ecs_taskdefinition: ensure cast to integer and fix idempotency of taskdefinition creation May 10, 2021

alinabuzachis changed the title ~~ecs_taskdefinition: ensure cast to integer and fix idempotency of taskdefinition creation~~ ecs_taskdefinition: ensure cast to integer and idempotency fix May 10, 2021

alinabuzachis mentioned this pull request May 11, 2021

ecs_taskdefinition: Integer parameters for maxSwap and swappiness are not converted from strings. #570

Closed

alinabuzachis force-pushed the ecs_taskdef_conv branch from 237156e to 6e9ea70 Compare May 11, 2021 21:52

markuman requested changes May 19, 2021

View reviewed changes

alinabuzachis requested review from tremble and jillr May 20, 2021 16:00

markuman approved these changes May 20, 2021

View reviewed changes

tremble reviewed Jul 7, 2021

View reviewed changes

jillr reviewed Jul 7, 2021

View reviewed changes

alinabuzachis added 4 commits July 13, 2021 14:12

ecs_taskdefinition: Fix some parameters cast to int

4b52107

Signed-off-by: Alina Buzachis <[email protected]>

Fix idempotence

b0328c8

Signed-off-by: Alina Buzachis <[email protected]>

Add more casts to int

24a9d47

Signed-off-by: Alina Buzachis <[email protected]>

Add changelog fragment

5ec234e

Signed-off-by: Alina Buzachis <[email protected]>

alinabuzachis force-pushed the ecs_taskdef_conv branch from 6e9ea70 to 5ec234e Compare July 13, 2021 12:22

ansibullbot removed the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Jul 13, 2021

ansibullbot added community_review and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR needs_triage labels Jul 13, 2021

markuman approved these changes Jul 19, 2021

View reviewed changes

markuman requested review from jillr and tremble July 19, 2021 19:09

markuman mentioned this pull request Jul 20, 2021

Sanitize parameters when comparing with existing ecs_taskdefinition #281

Closed

tremble added the gate label Aug 11, 2021

ansible-zuul bot approved these changes Aug 11, 2021

View reviewed changes

ansible-zuul bot merged commit ed8ad61 into ansible-collections:main Aug 11, 2021

markuman mentioned this pull request Aug 18, 2021

ecs_taskdefiniton port_mapping not idempotent when missing hostPort #690

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ecs_taskdefinition: ensure cast to integer and idempotency fix #574

ecs_taskdefinition: ensure cast to integer and idempotency fix #574

alinabuzachis commented May 10, 2021 •

edited

Loading

amackenz2048 commented May 11, 2021

alinabuzachis commented May 11, 2021 •

edited

Loading

amackenz2048 commented May 11, 2021

markuman left a comment

markuman May 19, 2021

alinabuzachis May 20, 2021

markuman May 19, 2021

alinabuzachis May 20, 2021

markuman May 19, 2021

markuman May 19, 2021

alinabuzachis May 20, 2021

markuman left a comment

markuman commented Jul 7, 2021 •

edited

Loading

ansibullbot commented Jul 7, 2021

tremble Jul 7, 2021

markuman Jul 7, 2021

jillr left a comment

markuman left a comment

ansible-zuul bot left a comment

ecs_taskdefinition: ensure cast to integer and idempotency fix #574

ecs_taskdefinition: ensure cast to integer and idempotency fix #574

Conversation

alinabuzachis commented May 10, 2021 • edited Loading

SUMMARY

ISSUE TYPE

COMPONENT NAME

amackenz2048 commented May 11, 2021

alinabuzachis commented May 11, 2021 • edited Loading

amackenz2048 commented May 11, 2021

markuman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markuman left a comment

Choose a reason for hiding this comment

markuman commented Jul 7, 2021 • edited Loading

ansibullbot commented Jul 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jillr left a comment

Choose a reason for hiding this comment

markuman left a comment

Choose a reason for hiding this comment

ansible-zuul bot left a comment

Choose a reason for hiding this comment

alinabuzachis commented May 10, 2021 •

edited

Loading

alinabuzachis commented May 11, 2021 •

edited

Loading

markuman commented Jul 7, 2021 •

edited

Loading