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

Allow CPU and Memory to be templatable #19263

Closed
wants to merge 1 commit into from

Conversation

nikhilo
Copy link

@nikhilo nikhilo commented Dec 12, 2016

Issue Type:
  • Bugfix Pull Request
Plugin Name:

ecs_taskdefinition

Ansible Version:
ansible 2.0.0.2
Summary:

Currently, the module does not allow the CPU and Memory parameters to be templatable. They must be integer/long. This makes the module unusable with variables. In other words, we would need to call the module for every task definition.
This change will allow the CPU and Memory to be templatable like so,

    containers:
      - name: "nikhil-test"
        image: "nikhil-test-nginx:{{deploy_tag}}"
        memory: "{{mycontainers.nginx.memory}}"
        cpu: "{{mycontainers.nginx.cpu}}"
Example output:

Error before change:

botocore.exceptions.ParamValidationError: Parameter validation failed:
Invalid type for parameter containerDefinitions[0].memory, value: 1920, type: <type 'str'>, valid types: <type 'int'>, <type 'long'>
Invalid type for parameter containerDefinitions[0].cpu, value: 2560, type: <type 'str'>, valid types: <type 'int'>, <type 'long'>

This error is not seen with suggested change

Note

This is a clone of ansible/ansible-modules-extras#1715
I could not rebase the original PR due to losing access to the repo I originally contributed from

@simplesteph
Copy link
Contributor

Hi, can you please include the following changes?
They were suggested by @rmorlok as feedback.

https://github.com/ansible/ansible-modules-extras/pull/3389/files

Thanks

@ansibot ansibot added affects_2.3 This issue/PR affects Ansible v2.3 bugfix_pullrequest module This issue/PR relates to a module. labels Dec 13, 2016
@bcoca
Copy link
Member

bcoca commented Dec 13, 2016

you can just do:

    containers:
      - name: "nikhil-test"
        image: "nikhil-test-nginx:{{deploy_tag}}"
        memory: "{{mycontainers.nginx.memory|int}}"
        cpu: "{{mycontainers.nginx.cpu|int}}"

@simplesteph
Copy link
Contributor

hi @bcoca

Please see why it wouldn't work with the following:
ansible/ansible-modules-extras#2641

Many PRs used to address this:
ansible/ansible-modules-extras#3284
ansible/ansible-modules-extras#3389

The underlying issue is:
#15249

@nikhilo please merge my changes in your PR (see my message above) so we can get this merged in. Otherwise I'll also create a PR which may bring some confusion

@gregdek don't know if you saw my ping yesterday, but I accept to be a maintainer for ecs_service and ecs_taskdefinition. See ansible/ansible-modules-extras#3135 (comment)

@kkarimi
Copy link
Contributor

kkarimi commented Dec 13, 2016

I believe this should already be fixed with #18999 ?

@simplesteph
Copy link
Contributor

@kkarimi You're right. I commented your PR as I have one small concern.

@nikhilo
Copy link
Author

nikhilo commented Dec 15, 2016

It seems to be fixed by #18999
Closing

@nikhilo nikhilo closed this Dec 15, 2016
@ansibot ansibot added bug This issue/PR relates to a bug. and removed bugfix_pull_request labels Mar 5, 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.3 This issue/PR affects Ansible v2.3 aws bug This issue/PR relates to a bug. cloud module This issue/PR relates to a module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants