-
Notifications
You must be signed in to change notification settings - Fork 345
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
ec2_instance: Fix launch template condition, handle launch template - default value for instance_type #587
ec2_instance: Fix launch template condition, handle launch template - default value for instance_type #587
Conversation
Hi @markuman, can you take please a look at this for #451, this change would
If this approach looks like a possible solution, I can work for adding few integration tests for the change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A changelog fragment is missing.
5465923
to
70f5ad8
Compare
70f5ad8
to
449d959
Compare
recheck |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, in fact I think it doesn't make sense to have a default here. IMO, I'd rather have a failing playbook just because I forgot to pick an instance type, instead of a default value :)
changelogs/fragments/587-ec2_instance-default-instance-type-launch-template.yml
Show resolved
Hide resolved
|
||
if not module.params.get('instance_type') and not module.params.get('launch_template'): | ||
module.deprecate("Default value instance_type has been deprecated, in the future you must set an instance_type or a launch_template", | ||
date='2023-01-01', collection_name='amazon.aws') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jillr Just an info, do we use date or collection version for deprecations?
@mandar242 Other than that, it looks good to me. Thank you for taking time to work on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alinabuzachis date, we don't expect to always have the same cadence of major collection releases
recheck |
Build failed.
|
recheck |
Build failed.
|
@mandar242 you can add the gate label to this PR when the sanity/ignores-2.9.txt file is updated and the tests are passing |
Build failed.
|
recheck |
Build succeeded.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Update README, CONTRIBUTING.md: freenode -> libera.chat Reviewed-by: Andrew Klychkov <[email protected]> https://github.com/Andersson007
SUMMARY
The
launch_template
option inec2_instance
has a broken condition.Also the
launch_template
option defaults theinstance_type
tot2.micro
if not specified and ignores theinstance_type
specified in thelaunch_template
as said in the issue #451.Fixes
ISSUE TYPE
COMPONENT NAME
ec2_instance
ADDITIONAL INFORMATION
The change does not break existing functionality as tested by the integration test run locally.
Related to the condition fix in community.aws: ansible-collections/community.aws#111
Ran the following playbook to verify the change.