add check_mode for elb_application_lb* & refactor integration tests #894
Conversation
ansibullbot
added
community_review
feature
|
Build succeeded.
|
There was a problem hiding this comment.
Thanks @jatorcasso. I left some initial comments. I will do a more accurate review.
|
Build succeeded.
|
|
Build failed (third-party-check pipeline) integration testing with
|
jatorcasso
changed the title
|
Build failed (third-party-check pipeline) integration testing with
|
|
Build succeeded.
|
jatorcasso
force-pushed
the
modules/elb_application_lb
branch
from
a8f50a8
to
fed58c4
Compare
|
Build failed (third-party-check pipeline) integration testing with
|
|
Build failed.
|
|
recheck |
|
Build failed.
|
|
Build succeeded (third-party-check pipeline).
|
|
Build succeeded.
|
There was a problem hiding this comment.
Other than that, it looks good to me. Thank you @jatorcasso. Let's wait for some other reviews from @marknet15 @markuman.
|
Build succeeded (third-party-check pipeline).
|
| - name: Ensure elb_application_lb_info supports check_mode | ||
| elb_application_lb_info: | ||
| register: elb_info | ||
| check_mode: yes |
There was a problem hiding this comment.
Not sure what others think but I didn't think check mode was really applicable for info modules as nothing really changes
There was a problem hiding this comment.
I know that all the modules should support check_mode. You're right, there is no change. _info modules are a particular case, but don't know about the story behind the check_mode here. According to the argument spec, it supports check mode.
There was a problem hiding this comment.
Yeah makes sense, guess they are indeed quite a particular case
There was a problem hiding this comment.
Yeah, I was basing that off a conversation with @jillr who basically said it should be there so nothing breaks if the user does --check on an entire playbook
|
Build succeeded (third-party-check pipeline).
|
|
Build succeeded.
|
There was a problem hiding this comment.
@jatorcasso Can you please retain the ALB naming in this set of modules and tests? ELBs are generally understood to mean v1/Classic Load Balancers (covered by amazon.aws.elb_classic_lb, the module formerly known as ec2_elb_lb). Only Application Load Balancers (elbv2) are covered by these modules.
https://docs.aws.amazon.com/elasticloadbalancing/index.html
The term ALB is a widely-used and understood convention for AWS users https://cloudacademy.com/blog/application-load-balancer-vs-classic-load-balancer/
@jillr changed to ALB where it made the most sense to me. Please let me know if I missed any, thanks! |
|
Build succeeded.
|
|
Build succeeded (gate pipeline).
|
…nsible-collections#894) add check_mode for elb_application_lb* & refactor integration tests SUMMARY Add check_mode support for elb_application_lb* & refactor integration tests. ISSUE TYPE Feature Pull Request COMPONENT NAME elb_application_lb elb_application_lb_info Reviewed-by: Alina Buzachis <None> Reviewed-by: Joseph Torcasso <None> Reviewed-by: Jill R <None> Reviewed-by: Mark Woolley <[email protected]> (cherry picked from commit 239136b)
…c default sg (#1025) Backport: elb_application_lb - check_mode support, alb attributes, vpc default sg SUMMARY Backport #894 #963 #971 manually to resolve conflicts. ISSUE TYPE Feature Pull Request COMPONENT NAME elb_application_lb Reviewed-by: Markus Bergholz <[email protected]> Reviewed-by: Alina Buzachis <None>
SUMMARY
Add check_mode support for elb_application_lb* & refactor integration tests.
ISSUE TYPE
COMPONENT NAME
elb_application_lb
elb_application_lb_info