r/spot_fleet_request - add import support #12767

DrFaust92 · 2020-04-10T19:35:33Z

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Release note for CHANGELOG:

resource_aws_spot_fleet_request: import support

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSSpotFleetRequest_'
--- PASS: TestAccAWSSpotFleetRequest_basic (300.87s)
--- PASS: TestAccAWSSpotFleetRequest_tags (348.01s)
--- PASS: TestAccAWSSpotFleetRequest_associatePublicIpAddress (337.30s)
--- PASS: TestAccAWSSpotFleetRequest_instanceInterruptionBehavior (268.08s)
--- PASS: TestAccAWSSpotFleetRequest_fleetType (330.00s)
--- PASS: TestAccAWSSpotFleetRequest_iamInstanceProfileArn (279.76s)
--- PASS: TestAccAWSSpotFleetRequest_changePriceForcesNewRequest (628.97s)
--- PASS: TestAccAWSSpotFleetRequest_updateTargetCapacity (961.76s)
--- PASS: TestAccAWSSpotFleetRequest_updateExcessCapacityTerminationPolicy (710.72s)
--- PASS: TestAccAWSSpotFleetRequest_lowestPriceAzOrSubnetInRegion (336.94s)
--- PASS: TestAccAWSSpotFleetRequest_lowestPriceAzInGivenList (441.51s)
--- PASS: TestAccAWSSpotFleetRequest_lowestPriceSubnetInGivenList (363.19s)
--- PASS: TestAccAWSSpotFleetRequest_multipleInstanceTypesInSameAz (295.60s)
--- PASS: TestAccAWSSpotFleetRequest_multipleInstanceTypesInSameSubnet (336.95s)
--- PASS: TestAccAWSSpotFleetRequest_overriddingSpotPrice (349.50s)
--- PASS: TestAccAWSSpotFleetRequest_withoutSpotPrice (354.03s)
--- PASS: TestAccAWSSpotFleetRequest_multipleInstancePools (345.03s)
--- PASS: TestAccAWSSpotFleetRequest_withWeightedCapacity (352.40s)
--- PASS: TestAccAWSSpotFleetRequest_withEBSDisk (326.94s)
--- PASS: TestAccAWSSpotFleetRequest_LaunchSpecification_EbsBlockDevice_KmsKeyId (244.07s)
--- PASS: TestAccAWSSpotFleetRequest_LaunchSpecification_RootBlockDevice_KmsKeyId (191.76s)
--- PASS: TestAccAWSSpotFleetRequest_withTags (292.46s)
--- PASS: TestAccAWSSpotFleetRequest_placementTenancyAndGroup (110.42s)
--- PASS: TestAccAWSSpotFleetRequest_WithELBs (390.80s)

--- FAIL: TestAccAWSSpotFleetRequest_diversifiedAllocation (381.59s) -- unrelated issue to this PR, probably..

--- FAIL: TestAccAWSSpotFleetRequest_WithTargetGroups (259.41s) -- fails because of lack of permission to delete ENIs on my side. should work after the adding it to state as part of PR

DrFaust92 · 2020-04-24T15:44:45Z

placing this under wip as rebase probably won't be clean after #12732

anGie44 · 2020-04-24T15:59:04Z

thanks for the heads-up @DrFaust92 :)

DrFaust92 · 2020-04-24T20:05:34Z

@anGie44 , removing WIP. I think the other PR is mostly done and stabilized

anGie44 · 2020-04-24T22:29:05Z

Code changes LGTM @DrFaust92! I'm just seeing test failures on my end w/re to on-destroy but still investigating as your changes here are unrelated

website/docs/r/spot_fleet_request.html.markdown

anGie44 · 2020-04-27T23:56:50Z

aws/resource_aws_spot_fleet_request_test.go

+				ResourceName:            resourceName,
+				ImportState:             true,
+				ImportStateVerify:       true,
+				ImportStateVerifyIgnore: []string{"wait_for_fulfillment"},


@DrFaust92 this test is an interesting one b/c a diversified allocation_strategy cannot be set with an instance_pools_to_use_count; the SDK returns InstancePoolsToUseCount option is only available with the lowestPrice allocation strategy if a user for example provides a value greater than 1 via the resource schema. in this case however, we avoid the error by using the default which results in not setting the spotFleetConfig.InstancePoolsToUseCount with this value, but the imported state has a nil value for instance_pools_to_use_count, explaining the test error

perhaps a schema change is in order to address the fact that the default value of instance_pools_to_use_count is technically not valid for cases where allocation_strategy is not lowest-price @bflad?

..though to avoid too much change and since the field is set with the ForceNew attribute, a possible work around could be to explicitly set instance_pools_to_use_count w/in resourceAwsSpotFleetRequestRead (for cases where the config object returns nil i.e. where allocation_strategy != lowest-price) to match the default expected value of 1

There are a few options in this scenario:

Add instance_pools_to_use_count to the ImportStateVerifyIgnore

Add d.Set("instance_pools_to_use_count", 1) in the import state function

Adjust the schema for instance_pools_to_use_count from Default: 1 to Computed: true to denote its multiple defaults (this however will require additional adjustments in the Create logic to check the allocation strategy in addition to the != 1)

The first option would just make the tests pass for the sake of passing (still representing an operator user experience problem after import). The second option would technically be a no-op change to the resource, since that would match the implicit Create behavior where the attribute Default is passed through to the state because d.Set() in Read is not overwriting the value. The third option would technically be the most correct given the API descriptions, however it would prevent that attribute from always being set as it is today, which represents a slight breaking change if any configurations are dependent on that behavior in attribute references.

I think I lean towards option two as the safest choice to implement import to match the existing behavior and a separate followup issue can be added to track fixing the attribute schema and its handling in Create, potentially saved for a future major version (unless its actually causing issues of course!).

ill address this with the second option as proposed

aws/resource_aws_spot_fleet_request.go

bflad · 2020-04-28T03:07:15Z

aws/resource_aws_spot_fleet_request_test.go

+				ResourceName:            resourceName,
+				ImportState:             true,
+				ImportStateVerify:       true,
+				ImportStateVerifyIgnore: []string{"wait_for_fulfillment"},


There are a few options in this scenario:

Add instance_pools_to_use_count to the ImportStateVerifyIgnore

Add d.Set("instance_pools_to_use_count", 1) in the import state function

Adjust the schema for instance_pools_to_use_count from Default: 1 to Computed: true to denote its multiple defaults (this however will require additional adjustments in the Create logic to check the allocation strategy in addition to the != 1)

The first option would just make the tests pass for the sake of passing (still representing an operator user experience problem after import). The second option would technically be a no-op change to the resource, since that would match the implicit Create behavior where the attribute Default is passed through to the state because d.Set() in Read is not overwriting the value. The third option would technically be the most correct given the API descriptions, however it would prevent that attribute from always being set as it is today, which represents a slight breaking change if any configurations are dependent on that behavior in attribute references.

I think I lean towards option two as the safest choice to implement import to match the existing behavior and a separate followup issue can be added to track fixing the attribute schema and its handling in Create, potentially saved for a future major version (unless its actually causing issues of course!).

Co-Authored-By: angie pinilla <angelinepinilla@gmail.com>

Co-Authored-By: Brian Flad <bflad417@gmail.com>

DrFaust92 · 2020-04-28T11:31:46Z

rebased + minor change for import

--- PASS: TestAccAWSSpotFleetRequest_basic (254.46s)
--- PASS: TestAccAWSSpotFleetRequest_tags (391.92s)
--- PASS: TestAccAWSSpotFleetRequest_associatePublicIpAddress (266.37s)
--- PASS: TestAccAWSSpotFleetRequest_launchTemplate (302.21s)
--- PASS: TestAccAWSSpotFleetRequest_launchTemplate_multiple (272.57s)
--- PASS: TestAccAWSSpotFleetRequest_launchTemplateWithOverrides (280.52s)
--- PASS: TestAccAWSSpotFleetRequest_launchTemplateToLaunchSpec (600.17s)
--- PASS: TestAccAWSSpotFleetRequest_launchSpecToLaunchTemplate (635.27s)
--- PASS: TestAccAWSSpotFleetRequest_instanceInterruptionBehavior (273.26s)
--- PASS: TestAccAWSSpotFleetRequest_fleetType (267.43s)
--- PASS: TestAccAWSSpotFleetRequest_iamInstanceProfileArn (263.05s)
--- PASS: TestAccAWSSpotFleetRequest_changePriceForcesNewRequest (680.87s)
--- PASS: TestAccAWSSpotFleetRequest_updateTargetCapacity (957.81s)
--- PASS: TestAccAWSSpotFleetRequest_updateExcessCapacityTerminationPolicy (691.51s)
--- PASS: TestAccAWSSpotFleetRequest_lowestPriceAzOrSubnetInRegion (273.25s)
--- PASS: TestAccAWSSpotFleetRequest_lowestPriceAzInGivenList (299.75s)
--- PASS: TestAccAWSSpotFleetRequest_lowestPriceSubnetInGivenList (381.62s)
--- PASS: TestAccAWSSpotFleetRequest_multipleInstanceTypesInSameAz (349.75s)
--- PASS: TestAccAWSSpotFleetRequest_multipleInstanceTypesInSameSubnet (356.21s)
--- PASS: TestAccAWSSpotFleetRequest_overriddingSpotPrice (294.95s)
--- PASS: TestAccAWSSpotFleetRequest_withoutSpotPrice (294.14s)
--- PASS: TestAccAWSSpotFleetRequest_diversifiedAllocation (354.14s)
--- PASS: TestAccAWSSpotFleetRequest_multipleInstancePools (376.60s)
--- PASS: TestAccAWSSpotFleetRequest_withWeightedCapacity (328.23s)
--- PASS: TestAccAWSSpotFleetRequest_withEBSDisk (273.60s)
--- PASS: TestAccAWSSpotFleetRequest_LaunchSpecification_EbsBlockDevice_KmsKeyId (173.64s)
--- PASS: TestAccAWSSpotFleetRequest_LaunchSpecification_RootBlockDevice_KmsKeyId (173.28s)
--- PASS: TestAccAWSSpotFleetRequest_withTags (260.02s)
--- PASS: TestAccAWSSpotFleetRequest_placementTenancyAndGroup (107.42s)
--- PASS: TestAccAWSSpotFleetRequest_WithELBs (349.21s)
--- PASS: TestAccAWSSpotFleetRequest_WithTargetGroups (519.11s)
--- PASS: TestAccAWSSpotFleetRequest_disappears (240.49s)

anGie44 · 2020-04-29T15:06:28Z

Confirmed output of acceptance tests in teamcity:

--- PASS: TestAccAWSSpotFleetRequest_launchTemplate_multiple (186.74s)
--- PASS: TestAccAWSSpotFleetRequest_launchTemplateWithOverrides (191.69s)
--- PASS: TestAccAWSSpotFleetRequest_launchTemplate (252.03s)
--- PASS: TestAccAWSSpotFleetRequest_lowestPriceAzOrSubnetInRegion (253.63s)
--- PASS: TestAccAWSSpotFleetRequest_associatePublicIpAddress (253.85s)
--- PASS: TestAccAWSSpotFleetRequest_instanceInterruptionBehavior (254.09s)
--- PASS: TestAccAWSSpotFleetRequest_tags (254.60s)
--- PASS: TestAccAWSSpotFleetRequest_basic (254.91s)
--- PASS: TestAccAWSSpotFleetRequest_iamInstanceProfileArn (255.25s)
--- PASS: TestAccAWSSpotFleetRequest_fleetType (256.21s)
--- PASS: TestAccAWSSpotFleetRequest_multipleInstanceTypesInSameSubnet (272.77s)
--- PASS: TestAccAWSSpotFleetRequest_lowestPriceSubnetInGivenList (274.83s)
--- SKIP: TestAccAWSSpotFleetRequest_WithInstanceStoreAmi (0.00s)
--- PASS: TestAccAWSSpotFleetRequest_overriddingSpotPrice (275.46s)
--- PASS: TestAccAWSSpotFleetRequest_multipleInstanceTypesInSameAz (276.25s)
--- PASS: TestAccAWSSpotFleetRequest_placementTenancyAndGroup (54.60s)
--- PASS: TestAccAWSSpotFleetRequest_lowestPriceAzInGivenList (355.53s)
--- PASS: TestAccAWSSpotFleetRequest_LaunchSpecification_EbsBlockDevice_KmsKeyId (155.55s)
--- PASS: TestAccAWSSpotFleetRequest_LaunchSpecification_RootBlockDevice_KmsKeyId (155.58s)
--- PASS: TestAccAWSSpotFleetRequest_withTags (268.51s)
--- PASS: TestAccAWSSpotFleetRequest_withEBSDisk (271.96s)
--- PASS: TestAccAWSSpotFleetRequest_launchSpecToLaunchTemplate (529.37s)
--- PASS: TestAccAWSSpotFleetRequest_updateExcessCapacityTerminationPolicy (532.67s)
--- PASS: TestAccAWSSpotFleetRequest_disappears (261.06s)
--- PASS: TestAccAWSSpotFleetRequest_withWeightedCapacity (310.33s)
--- PASS: TestAccAWSSpotFleetRequest_diversifiedAllocation (373.73s)
--- PASS: TestAccAWSSpotFleetRequest_withoutSpotPrice (380.62s)
--- PASS: TestAccAWSSpotFleetRequest_WithELBs (336.77s)
--- PASS: TestAccAWSSpotFleetRequest_changePriceForcesNewRequest (603.00s)
--- PASS: TestAccAWSSpotFleetRequest_multipleInstancePools (391.17s)
--- PASS: TestAccAWSSpotFleetRequest_WithTargetGroups (399.28s)
--- PASS: TestAccAWSSpotFleetRequest_updateTargetCapacity (738.10s)
--- FAIL: TestAccAWSSpotFleetRequest_launchTemplateToLaunchSpec (642.39s) -- unrelated to changes here (timeout sometimes not sufficient for deleting instances); test being tracking in [GH-13065]

bflad

LGTM (acceptance tests pass minus #13065 failures) 👍

ghost · 2020-05-01T01:15:01Z

This has been released in version 2.60.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

ghost · 2020-05-30T17:11:02Z

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!

DrFaust92 requested a review from a team April 10, 2020 19:35

DrFaust92 changed the title ~~[WIP]r/spot_fleet_request - add import support~~ r/spot_fleet_request - add import support Apr 10, 2020

DrFaust92 changed the title ~~r/spot_fleet_request - add import support~~ [WIP]r/spot_fleet_request - add import support Apr 24, 2020

DrFaust92 changed the title ~~[WIP]r/spot_fleet_request - add import support~~ r/spot_fleet_request - add import support Apr 24, 2020

DrFaust92 changed the title ~~r/spot_fleet_request - add import support~~ [WIP]r/spot_fleet_request - add import support Apr 24, 2020

anGie44 added enhancement Requests to existing resources that expand the functionality or scope. and removed needs-triage Waiting for first response or review from a maintainer. labels Apr 24, 2020

anGie44 self-assigned this Apr 24, 2020

DrFaust92 changed the title ~~[WIP]r/spot_fleet_request - add import support~~ r/spot_fleet_request - add import support Apr 24, 2020

anGie44 approved these changes Apr 24, 2020

View reviewed changes

anGie44 reviewed Apr 27, 2020

View reviewed changes

website/docs/r/spot_fleet_request.html.markdown Outdated Show resolved Hide resolved

anGie44 self-requested a review April 27, 2020 18:00

anGie44 reviewed Apr 27, 2020

View reviewed changes

bflad reviewed Apr 28, 2020

View reviewed changes

DrFaust92 and others added 4 commits April 28, 2020 10:23

add import support

c7ab68b

fmt fix

fca6993

fix typo

758d90c

Co-Authored-By: angie pinilla <angelinepinilla@gmail.com>

rebase import and add to new test cases

0f33994

ghost added size/XXL Managed by automation to categorize the size of a PR. and removed size/L Managed by automation to categorize the size of a PR. labels Apr 28, 2020

DrFaust92 force-pushed the r/spot_fleet_request_import branch from 23a569c to 0f33994 Compare April 28, 2020 07:39

ghost added size/L Managed by automation to categorize the size of a PR. and removed size/XXL Managed by automation to categorize the size of a PR. labels Apr 28, 2020

DrFaust92 and others added 3 commits April 28, 2020 10:40

Update aws/resource_aws_spot_fleet_request.go

25a2097

Co-Authored-By: Brian Flad <bflad417@gmail.com>

Update aws/resource_aws_spot_fleet_request.go

122f6b4

Co-Authored-By: Brian Flad <bflad417@gmail.com>

set "instance_pools_to_use_count" in import

be31df2

DrFaust92 requested a review from bflad April 28, 2020 20:57

bflad approved these changes Apr 29, 2020

View reviewed changes

anGie44 added this to the v2.60.0 milestone Apr 30, 2020

anGie44 merged commit c7220bc into hashicorp:master Apr 30, 2020

anGie44 added a commit that referenced this pull request Apr 30, 2020

update CHANGELOG for #12767

978deef

ghost locked and limited conversation to collaborators May 30, 2020

DrFaust92 deleted the r/spot_fleet_request_import branch June 23, 2020 11:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

r/spot_fleet_request - add import support #12767

r/spot_fleet_request - add import support #12767

DrFaust92 commented Apr 10, 2020 •

edited

Loading

DrFaust92 commented Apr 24, 2020

anGie44 commented Apr 24, 2020

DrFaust92 commented Apr 24, 2020

anGie44 commented Apr 24, 2020 •

edited

Loading

anGie44 Apr 27, 2020 •

edited

Loading

anGie44 Apr 28, 2020

anGie44 Apr 28, 2020 •

edited

Loading

bflad Apr 28, 2020

DrFaust92 Apr 28, 2020

bflad Apr 28, 2020

DrFaust92 commented Apr 28, 2020

anGie44 commented Apr 29, 2020 •

edited

Loading

bflad left a comment

ghost commented May 1, 2020

ghost commented May 30, 2020

r/spot_fleet_request - add import support #12767

r/spot_fleet_request - add import support #12767

Conversation

DrFaust92 commented Apr 10, 2020 • edited Loading

Community Note

DrFaust92 commented Apr 24, 2020

anGie44 commented Apr 24, 2020

DrFaust92 commented Apr 24, 2020

anGie44 commented Apr 24, 2020 • edited Loading

anGie44 Apr 27, 2020 • edited Loading

Choose a reason for hiding this comment

anGie44 Apr 28, 2020

Choose a reason for hiding this comment

anGie44 Apr 28, 2020 • edited Loading

Choose a reason for hiding this comment

bflad Apr 28, 2020

Choose a reason for hiding this comment

DrFaust92 Apr 28, 2020

Choose a reason for hiding this comment

bflad Apr 28, 2020

Choose a reason for hiding this comment

DrFaust92 commented Apr 28, 2020

anGie44 commented Apr 29, 2020 • edited Loading

bflad left a comment

Choose a reason for hiding this comment

ghost commented May 1, 2020

ghost commented May 30, 2020

DrFaust92 commented Apr 10, 2020 •

edited

Loading

anGie44 commented Apr 24, 2020 •

edited

Loading

anGie44 Apr 27, 2020 •

edited

Loading

anGie44 Apr 28, 2020 •

edited

Loading

anGie44 commented Apr 29, 2020 •

edited

Loading