-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
EC2 migration to the new architecture #202
Conversation
Co-Authored-By: Aboisier <aboisiermichaud@gmail.com>
…nto refactoring/aws-lambda-config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just left a couple of comments that are mainly details and not very important ones, I'm up for a merge! 🎆 Great job overall, special mention to your unit tests for AWS resources, very neat! 🥇
self[name] = resource | ||
|
||
def _parse_instance(self, raw_instance): | ||
instance = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we got an opportunity with this migration to be PEP 8 compliant and start defining dictionaries like the following:
instance = {
'id' = raw_instance['InstanceId'],
'reservation_id' = raw_instance['ReservationId'],
'monitoring_enabled' = raw_instance['Monitoring']['State'] == 'enabled',
'user_data' = self.facade.ec2.get_instance_user_data(self.scope['region'], id)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a discussion about that with @Remi05 last Thursday and we came to the conclusion it's probably a better idea to continue with the current implementation. There were a couple of reasons why, but here are the most important IMHO:
- Easier debugging: let's say raw_instance['InstanceId'] fails because the dict does not have this key. Using the PEP 8 convention, we would not know exactly what caused the crash.
- Add stuff iteratively: Sometimes, you want to add stuff iteratively. That's not possible with the PEP 8 convention unless you want to use a hybrid of the two notations, but then we might as well keep up with the current solution
self[name] = resource | ||
|
||
def _parse_security_group(self, raw_security_group): | ||
security_group = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above.
Co-Authored-By: Aboisier <aboisiermichaud@gmail.com>
This PR is a work in progress for refactoring the EC2 service, as part of the #183 issue.
I was doing a lot of copy-pasting in between the Lambdas service and this one, and between the facade methods, so I generalized some classes and methods.
EC2 has a pretty complex structure, but it seems like our new architecture holds up.
Stuff I generalized:
Regions
Fetches a list of all the regions and builds a
regions
dictionary with <region_id, empty_dictionary> key pairs. It also sets theregions_count
ScopedResource
Abstract class which fetches resources in a given scope. The child class needs to define a
get_resources_in_scope(scope)
method. The scope can be anything; a region, some VPC id, etc. It also needs to define aparse_resource(resource)
method which returns a (resource_id, resource) tuple. The ScopedResource class loops and sets the resources in its own dictionary.