Fixes #23 #25
Fixes #23 #25
Conversation
I also found having the VpcId in the output useful, so I took the liberty to add it. I appreciate this might be a breaking change if somebody is depending on the output of this tool.
There was a problem hiding this comment.
I got an error on the csv output:
Traceback (most recent call last):
File "/Users/analogue/.pyenv/versions/3.7.0/bin/ec2-security-groups-dumper", line 11, in <module>
load_entry_point('ec2-security-groups-dumper', 'console_scripts', 'ec2-security-groups-dumper')()
File "/Users/analogue/src/ec2-security-groups-dumper/ec2_security_groups_dumper/main.py", line 390, in main
print(firewall.csv)
File "/Users/analogue/src/ec2-security-groups-dumper/ec2_security_groups_dumper/main.py", line 281, in csv
for fr in self.rules:
File "/Users/analogue/src/ec2-security-groups-dumper/ec2_security_groups_dumper/main.py", line 224, in rules
main_row['vpc_id'])
File "/Users/analogue/src/ec2-security-groups-dumper/ec2_security_groups_dumper/main.py", line 66, in __init__
assert isinstance(vpc_id, str)
AssertionError
|
I've pushed a minor tweak, the JSON output was incorrect. I'm a bit confused about that AssertionError, what is the AWS API returning for The only thing I can think of is maybe you have some Security Groups for the pre VPC era and the VpcId field is being returned as |
|
Yes, I have some old classic stuff around, and in this case, the |
|
I'm not sure how you'd prefer this fixed, I've pushed a commit which works around the lack of the |
| @@ -61,6 +63,7 @@ def __init__(self, | |||
| assert isinstance(id, str), "Invalid id: {}".format(id) | |||
| assert isinstance(name, str) | |||
| assert isinstance(description, str) | |||
| assert isinstance(vpc_id, str) | |||
There was a problem hiding this comment.
Taking classic into account, a firewall rule does not need to be in a VPC, so let's make this str or NoneType.
| @@ -36,6 +36,7 @@ def __init__(self, | |||
| id, | |||
| name, | |||
| description, | |||
| vpc_id, | |||
There was a problem hiding this comment.
make it default to None to support classic.
| @@ -175,6 +180,7 @@ def rules(self): | |||
| main_row['id'], | |||
| main_row['name'], | |||
| main_row['description'], | |||
| main_row['vpc_id'], | |||
There was a problem hiding this comment.
You can use a named param as vpc_id is optional.
| @@ -189,6 +195,7 @@ def rules(self): | |||
| main_row['id'], | |||
| main_row['name'], | |||
| main_row['description'], | |||
| main_row['vpc_id'], | |||
| @@ -204,6 +211,7 @@ def rules(self): | |||
| main_row['id'], | |||
| main_row['name'], | |||
| main_row['description'], | |||
| main_row['vpc_id'], | |||
| @@ -212,14 +220,16 @@ def rules(self): | |||
| else: | |||
| fr = FirewallRule(main_row['id'], | |||
| main_row['name'], | |||
| main_row['description']) | |||
| main_row['description'], | |||
| main_row['vpc_id']) | |||
| @@ -301,6 +313,7 @@ def _get_rules_from_aws(self): | |||
| group_dict['id'] = group['GroupId'] | |||
| group_dict['name'] = group['GroupName'] | |||
| group_dict['description'] = group.get('Description', None) | |||
| group_dict['vpc_id'] = group.get('VpcId', "") | |||
There was a problem hiding this comment.
Like the optional fields use None instead of an empty string.
I also found having the VpcId in the output useful, so I took the
liberty to add it.
I appreciate this might be a breaking change if somebody is depending on
the output of this tool.