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

[PR #592/a2be6da7 backport][stable-2] elb_classic_lb: fix - resolve security_group_ids when providing security_group_names #594

Conversation

patchback[bot]
Copy link

@patchback patchback bot commented Dec 21, 2021

This is a backport of PR #592 as merged into main (a2be6da).

SUMMARY

Fix failed to resolve security_group_ids when providing security_group_names.
Fix broken tasks in integration tests.

Fixes #589.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

elb_classic_lb

…ity_group_names (#592)

elb_classic_lb: fix - resolve security_group_ids when providing security_group_names

SUMMARY

Fix failed to resolve security_group_ids when providing security_group_names.
Fix broken tasks in integration tests.
Fixes #589.

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

elb_classic_lb

Reviewed-by: Mark Chappell <None>
Reviewed-by: None <None>
(cherry picked from commit a2be6da)
@jillr jillr added the gate label Jan 12, 2022
Copy link
Contributor

@ansible-zuul ansible-zuul bot left a comment

Choose a reason for hiding this comment

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

LGTM!

@ansible-zuul ansible-zuul bot merged commit 03b80e2 into stable-2 Jan 12, 2022
@ansible-zuul ansible-zuul bot deleted the patchback/backports/stable-2/a2be6da7f4169527493ab2ec4ff061eb4d85ac7b/pr-592 branch January 12, 2022 23:49
@andrew-pickin-epi
Copy link

This fix doesn't work, if the ELB doesn't exist.
self.elb is this None and get() fails.

The suggested fix in #589 does actually work.

Copy link

@andrew-pickin-epi andrew-pickin-epi left a comment

Choose a reason for hiding this comment

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

This doesn NOT fix the issue in question if the ELB doesn't already exist.

@@ -744,7 +744,7 @@ def __init__(self, module):
if security_group_names:
# Use the subnets attached to the VPC to find which VPC we're in and
# limit the search
if self.elb.get('Subnets'):
if self.elb.get('Subnets', None):

Choose a reason for hiding this comment

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

Fails if ELB doesn't exsist (even if called with state: absent)
elb is undefined. get() throws error.

Replace with:
if self.elb is not None and self.elb.get('Subnets', None):

Copy link

@andrew-pickin-epi andrew-pickin-epi left a comment

Choose a reason for hiding this comment

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

The original problem is not 'Subnets' being absent, but elb.
This change does not fix this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants