-
Notifications
You must be signed in to change notification settings - Fork 330
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
#1210: EBSVolume => new data model, Allow node attr updates from multiple intel modules #1214
Conversation
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.
Looks great
|
||
|
||
@dataclass(frozen=True) | ||
class EBSVolumeInstanceProperties(CartographyNodeProperties): |
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.
Looking at the sample DESCRIBE_INSTANCES
, the only properties about Volumes that are returned are just volumeid
and deleteontermination
, right?
cartography/tests/data/aws/ec2/instances.py
Lines 390 to 395 in facb63b
'Ebs': { | |
'AttachTime': datetime.datetime(2018, 10, 14, 16, 30, 26), | |
'DeleteOnTermination': True, | |
'Status': 'attached', | |
'VolumeId': 'vol-04', | |
}, |
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.
Good point, will update
tests/integration/cartography/intel/aws/ec2/test_ec2_volumes.py
Outdated
Show resolved
Hide resolved
(r['n1.id'], r['n2.id']) for r in result | ||
|
||
# Assert that the account to volume rels exist | ||
assert check_rels( |
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.
We should maybe do this check after syncing instances, but before syncing the volumes
Co-authored-by: Ramon Petgrave <32398091+ramonpetgrave64@users.noreply.github.com>
…iple intel modules (#1214) See #1210 for full context. #1154 tried to solve this problem by updating the querybuilder but this was too complex and would not generalize well. This solution is simpler where we use different property classes for each API response so that we don't overwrite properties on a node set by another sync job. This PR can be reviewed commit-by-commit: - c0d9ac4 shows a repro of the error with a failing integration test. - facb63b shows the solution using multiple classes. --------- Co-authored-by: Ramon Petgrave <32398091+ramonpetgrave64@users.noreply.github.com>
Refactors EC2 Network Interfaces and Private IPs to the cartography data model. Fixes #1152 since the data model includes automatic cleanup jobs. - Uses same technique as #1214 to represent Network Interfaces as known by EC2 instances vs Network Interfaces known by describe-network-interfaces. - Also clearly marks that the Private IPs ingested here are known by Network Interfaces and not another source.
…multiple intel modules (lyft#1214) See lyft#1210 for full context. lyft#1154 tried to solve this problem by updating the querybuilder but this was too complex and would not generalize well. This solution is simpler where we use different property classes for each API response so that we don't overwrite properties on a node set by another sync job. This PR can be reviewed commit-by-commit: - c0d9ac4 shows a repro of the error with a failing integration test. - facb63b shows the solution using multiple classes. --------- Co-authored-by: Ramon Petgrave <32398091+ramonpetgrave64@users.noreply.github.com>
lyft#1219) Refactors EC2 Network Interfaces and Private IPs to the cartography data model. Fixes lyft#1152 since the data model includes automatic cleanup jobs. - Uses same technique as lyft#1214 to represent Network Interfaces as known by EC2 instances vs Network Interfaces known by describe-network-interfaces. - Also clearly marks that the Private IPs ingested here are known by Network Interfaces and not another source.
See #1210 for full context.
#1154 tried to solve this problem by updating the querybuilder but this was too complex and would not generalize well.
This solution is simpler where we use different property classes for each API response so that we don't overwrite properties on a node set by another sync job.
This PR can be reviewed commit-by-commit: