-
Notifications
You must be signed in to change notification settings - Fork 9.1k
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
spot_instance_request: fixed network interfaces configuration #4
Conversation
Hm, I can't see how it the test failure related to this PR. Previously it has passed test ok. |
Hi @hrach |
@radeksimko thanks a lot. I have wrongly modified the patch; fixed now. |
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.
Hi @hrach
thanks for the PR and sorry to keep you waiting here.
It would be great to have a test covering this bugfix to make sure we won't break it again in the future.
Basically you can copy one of the existing tests here https://github.com/terraform-providers/terraform-provider-aws/blob/master/aws/resource_aws_spot_instance_request_test.go#L15 and modify accordingly.
Then you can run the test against an AWS account like this:
AWS_PROFILE=yourprofile make testacc TESTARGS='-run= TestAccAWSSpotInstanceRequest_nameOfYourNewTest'
I hope the above makes sense.
I left you one question there regarding the implementation.
// be nil | ||
if len(instanceOpts.NetworkInterfaces) > 0 { | ||
spotOpts.LaunchSpecification.SecurityGroupIds = instanceOpts.NetworkInterfaces[0].Groups | ||
spotOpts.LaunchSpecification.SubnetId = instanceOpts.NetworkInterfaces[0].SubnetId |
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.
Is there any reason we can't just add this line here?
spotOpts.LaunchSpecification.NetworkInterfaces = instanceOpts.NetworkInterfaces
We don't seem to have a test covering the case in the comment here, but I'd be worried that removing the logic may break some unrelated edge cases... 🤔
Ok, repushed.
I'm definitely open to fix it as you wish, though, I have no idea how to make further inspection of aws-sdk to validate my points. |
Well, the bad thing is that the test also pass when I do not apply the fix :-O So probably I would need some guidance what and how should I test the real instance setting. |
That's a good point, although given the structure of the AWS SDK and the way it works it would has to be an API (server side) fix, because the SDK structures are pretty much all generated from API JSON/XML structures. There's rarely any (if any) custom parsing logic. Server-side bugfix is certainly what may have happened though 👍 I don't have too strong opinion on this, so as long as @catsby is happy as the original author of the linked PR, we're all happy. 😃
I think the reason the test is passing is because we're launching the instance into a subnet with auto-assignment enabled, as mentioned in this comment. If you set It's not necessary, but if you want you can also make the test more thorough by checking API response, e.g. adding a condition to
where you can check contents of As a side note it would be nice to separate this test instead of modifying an existing one so we can cover that specific use-case in isolation from others. Copy-pasting is totally aok in test cases. |
Merged in #1070 , thanks! |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks! |
Fixes configuration public ip for spot_instance_request. Originally hashicorp/terraform#13736 & bug hashicorp/terraform#9575.