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

Add tag filtering support for ELB (#45) #47

Conversation

kadaan
Copy link
Contributor

@kadaan kadaan commented Sep 3, 2019

Partial fix for #45
Add support for loading the tags for ELBs, enabling filtering on them.

return loadBalancers, nil
}

func (a *AWS) findElbTags(elbNames []string) ([]*elb.TagDescription, error) {
Copy link
Owner

@jckuester jckuester Sep 13, 2019

Choose a reason for hiding this comment

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

Thanks for your PR 👍 Not sure if you have seen that I have some generic function that tries to find tag fields by name (https://github.com/cloudetc/awsweeper/pull/47/files#diff-badd01dec4bab337623be10dca5e352cR136) in the output (https://github.com/cloudetc/awsweeper/blob/master/resource/resource.go#L81).

Not sure if that can be extended to work for ELB (or if tags in ELB outputs structs cannot be easily found). Anyway, I will have a look and come back to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The elb tags are not returned in the first call and have to be requested separately.

Copy link
Owner

@jckuester jckuester Sep 13, 2019

Choose a reason for hiding this comment

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

Ah, I see, makes sense. We can enable the integration-test then, which I skipped so far: https://github.com/cloudetc/awsweeper/blob/master/test/elb_test.go#L20

@@ -307,7 +313,44 @@ func (a *AWS) elbs() (interface{}, error) {
if err != nil {
return nil, err
}
return output.LoadBalancerDescriptions, nil
loadBalancerNames := make([]string, len(output.LoadBalancerDescriptions))
Copy link
Owner

@jckuester jckuester Sep 17, 2019

Choose a reason for hiding this comment

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

Your PR looks good, but I will create some unit tests in supported_test.go, as we now have quite some logic here, and matching wrong tags with wrong ELBs would really cause some bad user experience (even though, I don't see a problem in your code right now). Sorry, I am little bit conservative here, because this tool deletes stuff :)

@jckuester jckuester changed the base branch from master to feature-kadaan/elb-tag-filtering September 22, 2019 09:24
@jckuester jckuester merged commit 05c8133 into jckuester:feature-kadaan/elb-tag-filtering Sep 22, 2019
@kadaan kadaan deleted the Elb_Tag_Filtering_(#45) branch September 23, 2019 05:45
@jckuester jckuester mentioned this pull request Nov 14, 2019
jckuester added a commit that referenced this pull request Nov 21, 2019
* Add tag filtering support for ELB (#47)

* Enable tags integration test for ELB
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.

2 participants