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.

Sign up for GitHub

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

Jump to bottom

Fully support mixed instance policy #232

Merged
merged 6 commits into from
Mar 31, 2021
Merged

Fully support mixed instance policy #232

merged 6 commits into from
Mar 31, 2021

Conversation

JacobHenner
Copy link
Contributor

SUMMARY

Previously, setting instances_distribution was not supported.
instances_distribution should be supported, to allow users to enable
spot instances within their mixed instance ASGs.

Note: The type and significance of the mixed_instance_policy has
changed. It now captures all of the mixed_instance_policy configuration
parameters, rather than just a list of instance types.

Fixes #231

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

ec2_asg

@JacobHenner
Copy link
Contributor Author

I just realized tests were not updated - handling that now.

@JacobHenner JacobHenner force-pushed the feature-231 branch 4 times, most recently from 6baee97 to 91e4cc2 Compare September 16, 2020 17:21
@JacobHenner
Copy link
Contributor Author

It looks like some shippable tests have failed due to issues pulling docker images.

tremble
tremble requested changes Sep 17, 2020
View reviewed changes
Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to submit this change. I've retriggered the failed CI tests and they're now passing.

Mostly nits:

  • Please add some additional tasks to the integration tests rather than updating the existing tasks: it's important that we know that the existing behaviour doesn't change and break as people move between versions.
  • Please add a changelog fragment: https://docs.ansible.com/ansible/latest/community/development_process.html#changelogs-how-to
  • Unfortunately I think we need to use a different name for the new return data, switching from a list to a dict will break existing playbooks. (full_mixed_instances_policy ?) (@jillr @garethr any opinions?)

plugins/modules/ec2_asg.py Outdated Show resolved Hide resolved
@JacobHenner
Copy link
Contributor Author

JacobHenner commented Sep 17, 2020 •
edited
Loading

Thanks for the feedback. I'll make the requested adjustments and submit for review,

@ansibullbot
Copy link

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

@ansibullbot ansibullbot added affects_2.10 community_review feature This issue/PR relates to a feature request integration tests/integration module module needs_triage new_contributor Help guide this first time contributor plugins plugin (any type) tests tests labels Sep 18, 2020
JacobHenner added a commit to JacobHenner/community.aws that referenced this pull request Sep 21, 2020
@JacobHenner
Restore mixed_instances_policy backwards-compat
63faea0 
Restore mixed_instances_policy backwards compatibility by using
mixed_instances_policy_full to return full dictionary.

Also, fix some doc typos, add CHANGELOG fragment, and split into
separate test case.

Addresses feedback in ansible-collections#232
@JacobHenner JacobHenner requested a review from tremble September 21, 2020 17:06
@ansibullbot ansibullbot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed community_review labels Sep 21, 2020
markuman
markuman approved these changes Oct 15, 2020
View reviewed changes
Copy link
Member

@markuman markuman left a comment

Choose a reason for hiding this comment

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

LGTM

@markuman
Copy link
Member

@JacobHenner can you resolve the change requst by @tremble?

tremble
tremble approved these changes Oct 15, 2020
View reviewed changes
Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

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

The code looks sound. I'd like @jillr or @s-hertel to have a quick look and do the merge if they're happy with the formatting.

plugins/modules/ec2_asg.py Outdated Show resolved Hide resolved
jillr
jillr reviewed Oct 22, 2020
View reviewed changes
Copy link
Collaborator

@jillr jillr left a comment

Choose a reason for hiding this comment

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

The code looks good but I haven't been able to test successfully, #230 is breaking the test suite and I haven't sorted that out yet. @tremble were you able to test this change?

plugins/modules/ec2_asg.py Outdated Show resolved Hide resolved
@ansibullbot ansibullbot added community_review stale_ci CI is older than 7 days, rerun before merging and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Nov 16, 2020
@JacobHenner @goneri
Fully support mixed instance policy
7bd3ac8 
Previously, setting instances_distribution was not supported.
instances_distribution should be supported, to allow users to enable
spot instances within their mixed instance ASGs.

Note: The type and significance of the mixed_instance_policy has
changed. It now captures all of the mixed_instance_policy configuration
parameters, rather than just a list of instance types.

Fixes ansible-collections#231
goneri pushed a commit to JacobHenner/community.aws that referenced this pull request Feb 12, 2021
@JacobHenner @goneri
Restore mixed_instances_policy backwards-compat
6548162 
Restore mixed_instances_policy backwards compatibility by using
mixed_instances_policy_full to return full dictionary.

Also, fix some doc typos, add CHANGELOG fragment, and split into
separate test case.

Addresses feedback in ansible-collections#232
@goneri
Copy link
Member

goneri commented Feb 12, 2021 •
edited
Loading

@JacobHenner, don't be surprised. I just rebased your PR :-).

jillr
jillr reviewed Feb 12, 2021
View reviewed changes
Copy link
Collaborator

@jillr jillr left a comment

Choose a reason for hiding this comment

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

Apologies this one stalled @JacobHenner, I think we just need to add back the import for camel_dict_to_snake_dict (I suspect it was removed in another commit now that this PR has been rebased). That should get the sanity tests passing so that we can merge.

@ansibullbot ansibullbot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed community_review labels Feb 12, 2021
tremble
tremble requested changes Feb 13, 2021
View reviewed changes
Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

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

A few minor bits and pieces. Let's see if we can get this over the finishing line

plugins/modules/ec2_asg.py Outdated Show resolved Hide resolved
plugins/modules/ec2_asg.py Show resolved Hide resolved
plugins/modules/ec2_asg.py Outdated Show resolved Hide resolved
plugins/modules/ec2_asg.py Show resolved Hide resolved
@ansibullbot ansibullbot added community_review and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Feb 13, 2021
tremble
tremble approved these changes Feb 13, 2021
View reviewed changes
Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

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

Ran the tests locally. ec2_launch_template is currently broken but once fixed the unsupported ec2_asg tests pass with this change.

JacobHenner and others added 4 commits February 13, 2021 17:24
@JacobHenner @tremble
Restore mixed_instances_policy backwards-compat
b894c05 
Restore mixed_instances_policy backwards compatibility by using
mixed_instances_policy_full to return full dictionary.

Also, fix some doc typos, add CHANGELOG fragment, and split into
separate test case.

Addresses feedback in ansible-collections#232
@JacobHenner @tremble
Only return mixed_instances_policy_full if set
9f8d82f
@tremble
Fix imports
c46bead
@tremble
use scrub_non_parameters helper
ef836c4
@tremble tremble requested a review from jillr February 13, 2021 16:25
tremble
tremble approved these changes Mar 31, 2021
View reviewed changes
Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

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

Noticed some missing version_added.

plugins/modules/ec2_asg.py Show resolved Hide resolved
plugins/modules/ec2_asg.py Show resolved Hide resolved
plugins/modules/ec2_asg.py Show resolved Hide resolved
plugins/modules/ec2_asg.py Show resolved Hide resolved
plugins/modules/ec2_asg.py Show resolved Hide resolved
plugins/modules/ec2_asg.py Show resolved Hide resolved
plugins/modules/ec2_asg.py Show resolved Hide resolved
@tremble tremble merged commit 95ecf7d into ansible-collections:main Mar 31, 2021
@tremble
Copy link
Contributor

tremble commented Mar 31, 2021

Thanks for your time and effort on this.

Sorry it's taken a while to get this merged.

alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request Jul 19, 2021
@JacobHenner
Fully support mixed instance policy (ansible-collections#232)
0fdb95e 
* Fully support mixed instance policy

Previously, setting instances_distribution was not supported.
instances_distribution should be supported, to allow users to enable
spot instances within their mixed instance ASGs.

Note: The type and significance of the mixed_instance_policy has
changed. It now captures all of the mixed_instance_policy configuration
parameters, rather than just a list of instance types.

Fixes ansible-collections#231

* Restore mixed_instances_policy backwards-compat

Restore mixed_instances_policy backwards compatibility by using
mixed_instances_policy_full to return full dictionary.

Also, fix some doc typos, add CHANGELOG fragment, and split into
separate test case.

Addresses feedback in ansible-collections#232

* Only return mixed_instances_policy_full if set
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request Jul 19, 2021
@JacobHenner
Fully support mixed instance policy (ansible-collections#232)
5533099 
* Fully support mixed instance policy

Previously, setting instances_distribution was not supported.
instances_distribution should be supported, to allow users to enable
spot instances within their mixed instance ASGs.

Note: The type and significance of the mixed_instance_policy has
changed. It now captures all of the mixed_instance_policy configuration
parameters, rather than just a list of instance types.

Fixes ansible-collections#231

* Restore mixed_instances_policy backwards-compat

Restore mixed_instances_policy backwards compatibility by using
mixed_instances_policy_full to return full dictionary.

Also, fix some doc typos, add CHANGELOG fragment, and split into
separate test case.

Addresses feedback in ansible-collections#232

* Only return mixed_instances_policy_full if set
danielcotton pushed a commit to danielcotton/community.aws that referenced this pull request Nov 23, 2021
@JacobHenner @danielcotton
Fully support mixed instance policy (ansible-collections#232)
dda1bc8 
* Fully support mixed instance policy

Previously, setting instances_distribution was not supported.
instances_distribution should be supported, to allow users to enable
spot instances within their mixed instance ASGs.

Note: The type and significance of the mixed_instance_policy has
changed. It now captures all of the mixed_instance_policy configuration
parameters, rather than just a list of instance types.

Fixes ansible-collections#231

* Restore mixed_instances_policy backwards-compat

Restore mixed_instances_policy backwards compatibility by using
mixed_instances_policy_full to return full dictionary.

Also, fix some doc typos, add CHANGELOG fragment, and split into
separate test case.

Addresses feedback in ansible-collections#232

* Only return mixed_instances_policy_full if set
@mandar242 mandar242 mentioned this pull request Mar 29, 2022
Closed
@tremble tremble mentioned this pull request Apr 19, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markuman markuman markuman approved these changes

@tremble tremble tremble approved these changes

@jillr jillr Awaiting requested review from jillr

Assignees
No one assigned
Labels
affects_2.10 community_review feature This issue/PR relates to a feature request integration tests/integration module module new_contributor Help guide this first time contributor plugins plugin (any type) stale_ci CI is older than 7 days, rerun before merging tests tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ec2_asg mixed_instance_policy does not support instances_distribution
7 participants
@JacobHenner @ansibullbot @markuman @goneri @tremble @jillr @gravesm