ec2_instance: Add count parameter to ec2_instance #539

mandar242 · 2021-10-19T08:30:19Z

SUMMARY

UPDATE: Added count and exact_count parameters to ec2_instance instead of min_count and max_count.

Adding min_count and max_count parameter to ec2_instance.

Fixes #536.

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

ec2_instance

ansibullbot · 2021-10-19T08:30:53Z

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

ansibullbot · 2021-10-25T20:15:47Z

@mandar242 this PR contains the following merge commits:

2cc6c81

Please rebase your branch to remove these commits.

click here for bot help

jillr

Good work, thanks @mandar242!

plugins/modules/ec2_instance.py

tremble

Thanks for this @mandar242

plugins/modules/ec2_instance.py

tests/integration/targets/ec2_instance/roles/ec2_instance/tasks/instance_multiple.yml

tremble

Hi @mandar242,
(following up on a private conversation so the reasoning behind any new logic can be publicly followed)

Usually, with Ansible we try to default to things being idempotent, and update an existing object rather than always triggering a change. Given this context the way I would generally have interpreted "Max" and "Min" for the ec2_instance module is that we want the total number of matching instances to be between the two values: something approximating an Ansible-managed alternative to an AutoScalingGroup.

Currently, we have users migrating from the old ec2 module [1] which has exact_count and count. count would always spawn instances (the behaviour you've implemented), but exact_count would try to ensure that the total number of matching (filters) instances is exact_count (the behaviour I thought you were trying to implement).

The fact that I misinterpreted what you had tried to implement things (having generally used exact_count rather than count) means that users probably will too.

Rather than exposing max_count and min_count, I'd recommend:

count (int - default 1): number of instances expected to match the filters, mimicking ec2.py's "exact_count" by default.
always_launch (bool - default false): if set to true then always trigger a launch. (as you do today if count > 1)

This means that the count=1 behaviour is much closer to the count>1 behaviour, it also means if the user really did want to always launch a new instance, they can now also launch them one at a time if they wanted.

plugins/modules/ec2_instance.py

tests/integration/targets/ec2_instance/roles/ec2_instance/tasks/instance_multiple.yml

plugins/modules/ec2_instance.py

tremble · 2021-11-24T12:27:43Z

plugins/modules/ec2_instance.py

+
+        elif current_count < exact_count:
+            to_launch = exact_count - current_count
+            module.params['to_launch'] = to_launch


We could overwrite module.params['count'] here to avoid the extra code path in build_run_instance_spec

@jillr do you have any opinions on this?

I generally don't like overwriting module.params, I worry that it's asking for issues down the road when we make some other change that uses count and expects it to be whatever the user originally specified. I think I'd rather have the extra conditional?

plugins/modules/ec2_instance.py

jillr

LGTM, thanks for all your work on this @mandar242!

jillr · 2021-11-29T23:35:29Z

plugins/modules/ec2_instance.py

+
+        elif current_count < exact_count:
+            to_launch = exact_count - current_count
+            module.params['to_launch'] = to_launch


I generally don't like overwriting module.params, I worry that it's asking for issues down the road when we make some other change that uses count and expects it to be whatever the user originally specified. I think I'd rather have the extra conditional?

ansible-zuul

LGTM!

patchback · 2021-11-30T08:06:17Z

Backport to stable-2: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-2/760186c5c576548cdb4bb7a4e2f6a79d9c1f3b44/pr-539

Backported as #580

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

ec2_instance: Add count parameter to ec2_instance SUMMARY Adding min_count and max_count parameter to ec2_instance. Fixes #536. ISSUE TYPE Feature Pull Request COMPONENT NAME ec2_instance Reviewed-by: Jill R <None> Reviewed-by: Mark Chappell <None> Reviewed-by: Mandar Kulkarni <mandar242@gmail.com> Reviewed-by: None <None> (cherry picked from commit 760186c)

Add missing version_added to ec2_instance.py SUMMARY Realised after #539 merged that we didn't add version_added ISSUE TYPE Docs Pull Request COMPONENT NAME ec2_instance ADDITIONAL INFORMATION Reviewed-by: Alina Buzachis <None> Reviewed-by: None <None>

ec2_instance: Add count parameter to ec2_instance SUMMARY Adding min_count and max_count parameter to ec2_instance. Fixes #536. ISSUE TYPE Feature Pull Request COMPONENT NAME ec2_instance Reviewed-by: Jill R <None> Reviewed-by: Mark Chappell <None> Reviewed-by: Mandar Kulkarni <mandar242@gmail.com> Reviewed-by: None <None> (cherry picked from commit 760186c) Co-authored-by: Mandar Kulkarni <mandar242@gmail.com>

Add missing version_added to ec2_instance.py SUMMARY Realised after #539 merged that we didn't add version_added ISSUE TYPE Docs Pull Request COMPONENT NAME ec2_instance ADDITIONAL INFORMATION Reviewed-by: Alina Buzachis <None> Reviewed-by: None <None> (cherry picked from commit cf2b9aa)

Add missing version_added to ec2_instance.py SUMMARY Realised after #539 merged that we didn't add version_added ISSUE TYPE Docs Pull Request COMPONENT NAME ec2_instance ADDITIONAL INFORMATION Reviewed-by: Alina Buzachis <None> Reviewed-by: None <None> (cherry picked from commit cf2b9aa) Co-authored-by: Mark Chappell <mchappel@redhat.com>

…ble-jobs_29701 disable shippable jobs Reviewed-by: https://github.com/apps/ansible-zuul

ansibullbot added WIP Work in progress feature This issue/PR relates to a feature request integration tests/integration module module needs_triage plugins plugin (any type) tests tests labels Oct 19, 2021

jillr removed the needs_triage label Oct 19, 2021

mandar242 changed the title ~~[WIP] ec2_instance: Add count parameter to ec2_instance~~ ec2_instance: Add count parameter to ec2_instance Oct 25, 2021

ansibullbot added community_review and removed WIP Work in progress labels Oct 25, 2021

ansibullbot added merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html and removed community_review labels Oct 25, 2021

mandar242 added 5 commits October 25, 2021 13:22

Add count parameter to specify number of instances to launch

cea17fd

Add min_count, max_count, integration tests

f60c6a7

Correcting example params

a5ec52a

Add another example

c17d238

Add changelogs fragment

9142575

mandar242 force-pushed the ec2_instance_count_param branch from aec9a0a to 9142575 Compare October 25, 2021 20:26

ansibullbot added community_review and removed merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Oct 25, 2021

jillr reviewed Oct 25, 2021

View reviewed changes

plugins/modules/ec2_instance.py Outdated Show resolved Hide resolved

plugins/modules/ec2_instance.py Outdated Show resolved Hide resolved

plugins/modules/ec2_instance.py Outdated Show resolved Hide resolved

Modified based on feedback

e9fff0a

tremble requested changes Oct 26, 2021

View reviewed changes

Modified based on feedback

7be7807

tremble requested changes Nov 8, 2021

View reviewed changes

mandar242 added 3 commits November 12, 2021 13:33

minor fix, check_mode for enforce count, clean debug statements

fe37a6d

Modified based on feedback, added integration tests

9634fd5

Minor fix: sanity

d518e39

mandar242 changed the title ~~[WIP] ec2_instance: Add count parameter to ec2_instance~~ ec2_instance: Add count parameter to ec2_instance Nov 15, 2021

jillr reviewed Nov 15, 2021

View reviewed changes

plugins/modules/ec2_instance.py Outdated Show resolved Hide resolved

plugins/modules/ec2_instance.py Outdated Show resolved Hide resolved

tests/integration/targets/ec2_instance/roles/ec2_instance/tasks/instance_multiple.yml Show resolved Hide resolved

mandar242 added 2 commits November 15, 2021 16:28

Modified based on feedback

aae4f2f

Minor cleanup

1a0819e

mandar242 requested a review from tremble November 16, 2021 18:35

mandar242 requested a review from jillr November 24, 2021 04:53

tremble requested changes Nov 24, 2021

View reviewed changes

Modified based on feedback

b3fed8d

tremble approved these changes Nov 26, 2021

View reviewed changes

jillr approved these changes Nov 29, 2021

View reviewed changes

jillr added the gate label Nov 29, 2021

ansible-zuul bot approved these changes Nov 29, 2021

View reviewed changes

tremble added gate and removed gate labels Nov 30, 2021

ansible-zuul bot merged commit 760186c into ansible-collections:main Nov 30, 2021

tremble added the backport-2 PR should be backported to the stable-2 branch label Nov 30, 2021

patchback bot mentioned this pull request Nov 30, 2021

[PR #539/760186c5 backport][stable-2] ec2_instance: Add count parameter to ec2_instance #580

Merged

tremble mentioned this pull request Nov 30, 2021

Add missing version_added to ec2_instance.py #581

Merged

patchback bot mentioned this pull request Dec 3, 2021

[PR #581/cf2b9aa8 backport][stable-2] Add missing version_added to ec2_instance.py #585

Merged

mandar242 deleted the ec2_instance_count_param branch December 26, 2021 16:40

abikouo pushed a commit to abikouo/amazon.aws that referenced this pull request Oct 24, 2023

Merge pull request ansible-collections#539 from goneri/disable-shippa…

752d3e7

…ble-jobs_29701 disable shippable jobs Reviewed-by: https://github.com/apps/ansible-zuul

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ec2_instance: Add count parameter to ec2_instance #539

ec2_instance: Add count parameter to ec2_instance #539

mandar242 commented Oct 19, 2021 •

edited

Loading

ansibullbot commented Oct 19, 2021

ansibullbot commented Oct 25, 2021

jillr left a comment

tremble left a comment

tremble left a comment •

edited

Loading

tremble Nov 24, 2021

jillr Nov 29, 2021

jillr left a comment

jillr Nov 29, 2021

ansible-zuul bot left a comment

patchback bot commented Nov 30, 2021 •

edited

Loading

ec2_instance: Add count parameter to ec2_instance #539

ec2_instance: Add count parameter to ec2_instance #539

Conversation

mandar242 commented Oct 19, 2021 • edited Loading

SUMMARY

ISSUE TYPE

COMPONENT NAME

ansibullbot commented Oct 19, 2021

ansibullbot commented Oct 25, 2021

jillr left a comment

Choose a reason for hiding this comment

tremble left a comment

Choose a reason for hiding this comment

tremble left a comment • edited Loading

Choose a reason for hiding this comment

tremble Nov 24, 2021

Choose a reason for hiding this comment

jillr Nov 29, 2021

Choose a reason for hiding this comment

jillr left a comment

Choose a reason for hiding this comment

jillr Nov 29, 2021

Choose a reason for hiding this comment

ansible-zuul bot left a comment

Choose a reason for hiding this comment

patchback bot commented Nov 30, 2021 • edited Loading

Backport to stable-2: 💚 backport PR created

mandar242 commented Oct 19, 2021 •

edited

Loading

tremble left a comment •

edited

Loading

patchback bot commented Nov 30, 2021 •

edited

Loading