-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
ENI: DescribeNetworkInterfaces attachment.instance-id
filter
#3549
ENI: DescribeNetworkInterfaces attachment.instance-id
filter
#3549
Conversation
attachment.instance-id
filterattachment.instance-id
filter -- WIP
Thanks for the opening a PR @Preskton! Will convert it to a draft - let me know when you think it's ready for review. |
@bblommers since this is my first contribution, I have a couple of quick questions: Would you mind taking a quick peek at the change and let me know if a) the change is reasonable/directionally correct, and b) if the unit test appears in line with how other items are tested? Second, when I run |
This looks like the right approach @Preskton, both in terms of the actual change and the testing. Black shouldn't touch anything else, really, as it is already formatted in master. |
Thanks for taking a peek, @bblommers. Turns out my py installs were absolutely wrecked. Cleaned them up and was able to get things going correctly. I now have working tests - could you please provide feedback if this is an appropriate style? I was getting a little confused between if I should use client/resources or hit the Also, what guidance can you provide around clearing this warning? I'd like to knock it out before I merge.
Thanks in advance! |
attachment.instance-id
filter -- WIPattachment.instance-id
filter
attachment.instance-id
filterattachment.instance-id
filter
@mock_ec2_deprecated | ||
@mock_ec2 | ||
def test_elastic_network_interfaces_get_by_attachment_instance_id(): | ||
conn = boto.connect_vpc("the_key", "the_secret") |
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.
Creating the VPC can be done with the boto3-client as well.
@mock_ec2_deprecated
makes it possible to mock boto, but were trying to phase out support for boto at some point. So new tests shouldn't have any @.._deprecated
decorators.
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.
Do you have an example where we're using the new style and attaching an ENI? The only other ENI attach example I could find was using the deprecated style.
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.
It is be very similar using boto3. Haven't tested it out, but this should work:
eni1 = ec2_client.create_network_interface(SubnetId=..., Groups=[..])
ec2_client.attach_network_interface(DeviceIndex=1, InstanceId=..., NetworkInterfaceId=...)
) | ||
|
||
create_instances_result = ec2_resource.create_instances( | ||
ImageId="ami-d3adb33f", MinCount=1, MaxCount=1 |
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.
The warning pops up for quite a few tests - I think it's to do with the fact that this ImageId does not exist in our mocked world.
Don't worry this for now - we'll clean all those warnings up in one go (at some point..)
Codecov Report
@@ Coverage Diff @@
## master #3549 +/- ##
==========================================
+ Coverage 95.21% 95.22% +0.01%
==========================================
Files 535 535
Lines 59827 59844 +17
==========================================
+ Hits 56963 56989 +26
+ Misses 2864 2855 -9
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Hope you don't mind me touching this PR @Preskton - I just fixed up the test to get it ready |
This is now part of moto >= 2.2.17.dev13 |
Related issue: #3550
Description
Allows consumers to filter ENIs by the
attachment.instance-id
filter, if present. Today, attempting to filter byattachment.instance-id
yields an Exception similar to:TODO