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

[aws-garbage-collector] introduce new integration #84

Merged
merged 21 commits into from
Jun 3, 2019

Conversation

maorfr
Copy link
Contributor

@maorfr maorfr commented May 29, 2019

this integration does:

  • delete aws resources which do NOT have one of the following:
    • the resource name starts with the name of an existing IAM user
    • the resource name starts with stage or prod
    • the resource has one of these tags:
      • managed_by_integration - ignores resources managed by terraform-resources and terraform-users
      • owner - ignores some resources managed by terraform (HK)
      • aws_gc_hands_off - ignores manually managed resources

this integration does not:

  • support paging (planned for next iteration)

safety nets:

  • to actually perform the deletion, the flag enable-deletion should be set to true

supported aws resources:

  • s3
  • sqs
  • dynamodb
  • rds

Copy link
Contributor

@jmelis jmelis left a comment

Choose a reason for hiding this comment

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

The integration is great, and I don't see any major issues with the structure and workflow. I've given below some feedback, which summarizes in:

  • you seem to be capturing a lot of generic exceptions, I wish we could make it more robust instead. If you can try to improve things like this it would be great:
except KeyError as e:
    if 'Tags' in e.message:
  • there are some functions that can be refactored
  • minor style things like using list comprehensions more often etc
  • be more strict ith the exact naming patterns this integration will skip. doing if 'stag' in name seems a bit too generic, maybe? it's a hard problem to solve, though...

reconcile/cli.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
utils/aws_api.py Show resolved Hide resolved
utils/aws_api.py Show resolved Hide resolved
utils/aws_api.py Show resolved Hide resolved
utils/aws_api.py Show resolved Hide resolved
utils/aws_api.py Show resolved Hide resolved
utils/aws_api.py Outdated Show resolved Hide resolved
utils/aws_api.py Outdated Show resolved Hide resolved
utils/aws_api.py Outdated Show resolved Hide resolved
utils/aws_api.py Outdated Show resolved Hide resolved
utils/aws_api.py Outdated Show resolved Hide resolved
Copy link
Contributor

@jmelis jmelis left a comment

Choose a reason for hiding this comment

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

Awesome!!! 🚀

@maorfr maorfr merged commit 59757f2 into app-sre:master Jun 3, 2019
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