EC2 Instance sync => new data model #1146
Conversation
achantavy
force-pushed
the
ec2instancenewdatamodel
branch
from
e68b70f
to
44641bb
Compare
|
|
||
| for network_interface in instance['NetworkInterfaces']: | ||
| for security_group in network_interface['Groups']: | ||
| network_interface_list.append({ |
There was a problem hiding this comment.
@resilience-jychp - remember how we talked about cardinality? This is how I'm dealing with it in a transform function. In this case, an instance can have multiple network interfaces, and each of those can have multiple security groups, so I'll just add them each as their own entry to the list.
There was a problem hiding this comment.
@achantavy did the same, not totaly convinced it's the best way to do it, but definitively do the job
| InstanceId=instance_data['InstanceId'], | ||
| update_tag=update_tag, | ||
| ) | ||
| def transform_ec2_instances( |
There was a problem hiding this comment.
@ramonpetgrave64 - what you think about this big function, should I split it?
There was a problem hiding this comment.
I'd rather that return such a big tuple. Maybe convert it to a namedtuple, so we don't have to be so careful about the ordering?
It's a big one, but if we break it into many functions, we would be repeating a bit of code to dig deep into the objects. I'm okay with it either way.
| ) | ||
|
|
||
|
|
||
| def test_transform_and_load_ec2_tags(neo4j_session): | ||
| @patch.object(cartography.intel.aws.ec2.instances, 'get_ec2_instances', return_value=DESCRIBE_INSTANCES['Reservations']) |
There was a problem hiding this comment.
non-blocker: i feel like the patch belongs attached to _ensure_local_neo4j_has_test_ec2_instance_data . Maybe you can convert it to an autouse fixture, and do the patch within there.
There was a problem hiding this comment.
Hm, I'm going to defer this one, we can refactor the test in a follow-up.
| ) | ||
|
|
||
|
|
||
| def test_load_instance_information(neo4j_session): | ||
| @patch.object(cartography.intel.aws.ec2.instances, 'get_ec2_instances', return_value=DESCRIBE_INSTANCES['Reservations']) |
There was a problem hiding this comment.
nonblocker: same comment about patches
achantavy
force-pushed
the
ec2instancenewdatamodel
branch
from
44641bb
to
9487d38
Compare
Nah, this PR keeps all the fields the same so should be good. |
Fast follow of #1146 - removes duplicate calls to cleanup jobs. All these manual jobs will soon be deprecated as we refactor modules to the new data model.
Refactors EC2 instance sync to use the cartography data model. Adds previously missing tests. This is not a breaking change as the ID conventions are still the same as before (e.g. using EC2Instance instanceid for id instead of arn), even if they are inconsistent with other modules. A follow-up PR will standardize IDs using ARNs.
Fast follow of lyft#1146 - removes duplicate calls to cleanup jobs. All these manual jobs will soon be deprecated as we refactor modules to the new data model.
Refactors EC2 instance sync to use the cartography data model. Adds previously missing tests.
This is not a breaking change as the ID conventions are still the same as before (e.g. using EC2Instance instanceid for id instead of arn), even if they are inconsistent with other modules.
A follow-up PR will standardize IDs using ARNs.