Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Improved NetworkInterfaces() filtering support for ec2test #20
Conversation
axw
reviewed
Jan 20, 2015
| fatalf(400, "InvalidParameterValue", "describe ifaces: %v", err) | ||
| } | ||
| + _, requestedByID := idMap[i.Id] | ||
| + if len(idMap) == 0 { | ||
| + // since filter.ok() returns true when none matches, we |
axw
Jan 20, 2015
Member
This is confusing.
I think you mean that filter.ok() returns true when there are no filters specified, and so not specifying an ID means you don't implicitly filter by ID either.
I think this would be easier to understand, and would not require commentary, if you checked len(idMap) == 0 || idMap[i.Id] before everything else, continuing if the test fails.
dimitern
Jan 20, 2015
Member
Looking at it this morning it does look confusing :)
I've simplified the code to a single conditional: if filterMatch && (len(idMap) == 0 || idMap[i.Id] { ..., which works for all cases. Thanks!
axw
reviewed
Jan 20, 2015
| - ok, err := f.ok(i) | ||
| - _, known := idMap[i.Id] | ||
| - if ok && (len(idMap) == 0 || known) { | ||
| + // Without neither filers nor ids - return everything. |
dimitern
Jan 20, 2015
Member
D'oh! Nice catch, but I've simplified the code as you suggested below and this is now gone.
|
A couple of quibbles, but otherwise LGTM. |
dimitern commentedJan 19, 2015
Added most of the filters supported by DescribeNetworkInterfaces() to the ec2test server. None of the Elastic IP related filters or tags are supported by ec2test, but we can now write tests for NICs that filter by instance-id for example. Some changes to ec2test were needed to mimic EC2 behavior better.
The existing TestNetworkInterfaces test was refactored and reused for the new TestNetworkInterfacesFiltering test. Both tests share most of the setup code.