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

Rds elb tags #44

Merged
merged 2 commits into from May 3, 2016
Merged

Rds elb tags #44

merged 2 commits into from May 3, 2016

Conversation

jeffastorey
Copy link
Contributor

Added tag support for RDS and ELB - #35 and #40. Did these together since they were pretty similar.

Approach to get RDS ARN was to just grab a security group from the account and get the ARN as suggested http://stackoverflow.com/questions/32634402/work-with-rds-tags-in-boto3-how-to-get-arn-in-boto3. This is a bit hacky. I saw your note on maybe using an attribute for account - that could be a next pass, but I think the SG way will work for a first pass.

@jeffastorey
Copy link
Contributor Author

need to fix tests here

chunks = [elb_names[x:x+20] for x in xrange(0, len(elb_names), 20)]
fn = partial(self.process_tags, client=client)
with self.executor_factory(max_workers=3) as w:
tags = w.map(fn, chunks)
Copy link
Collaborator

Choose a reason for hiding this comment

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

there's a corner case here that makes it preferential to use the submit syntax instead of map, namely an elb being removed between the query of the elbs to querying its tags would fail the entire policy instead of the batch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will update

@jeffastorey
Copy link
Contributor Author

Comments addressed and rebased. Please review again when you can.

the result set.
"""
elb_names = [elb['LoadBalancerName'] for elb in elbs]
names_to_tags = {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

don't really think you need the names_to_tag map, you could just chunk the elbs and let the process_tags function directly update the elb, don't really need the result as its mutating a shared ref with transient ownership, just the exception processing.

that said there's requests for this now, so happy to cleanup after the fact post merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fair enough - thanks for reviewing

@kapilt
Copy link
Collaborator

kapilt commented May 3, 2016

lgtm, thanks

@kapilt kapilt merged commit aa3994b into cloud-custodian:master May 3, 2016
This was referenced May 3, 2016
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.

None yet

2 participants