-
Notifications
You must be signed in to change notification settings - Fork 9.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
Add FSx File Gateway Support to Storage Gateway & Add FSx Association resource #20082
Add FSx File Gateway Support to Storage Gateway & Add FSx Association resource #20082
Conversation
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.
Welcome @drewmullen 👋
It looks like this is your first Pull Request submission to the Terraform AWS Provider! If you haven’t already done so please make sure you have checked out our CONTRIBUTING guide and FAQ to make sure your contribution is adhering to best practice and has all the necessary elements in place for a successful approval.
Also take a look at our FAQ which details how we prioritize Pull Requests for inclusion.
Thanks again, and welcome to the community! 😃
I am also contributing this to this PR with Drew. |
does the acceptance test destroy phase work differently than a standard @AdamTylerLynch wrote some code for our acceptance tests print the hcl it assembles prior to building. We have successfully fully built & Working with local terraformwhen you run
Not working during acc testusing cloudtrail as an indicator, the acceptance tests do no appear to wait for the implicit dependency. notice that Error during acceptance test
|
Hi @drewmullen. By default, the acceptance testing framework calls out the Looking at the error message, my first guess would is that the FSx API can return several error codes when the resource is not found: you check for If that doesn't address it, can you run the test with the environment variable |
@gdavison howdy - thanks for your attention and response :D yes it works consistently outside acc tests which is what has really confused me (here is the hcl). would a low effort, appropriate test be to remove the content of the error message in **edit i tried this change and that worked! so the problem has to do with the Status error message capture. i asked around internally and the storagegateway team is recommending not to rely on error message because its actually just changed from "The specified file system association" to "Cannot find resource: ". you can actually see the different errors if you use different versions of the aws cli. i guess for now i should just add both errors using an or? |
I think it might be worthwhile looking at the nested error using So instead of using if tfawserr.ErrMessageContains(err, storagegateway.ErrCodeInvalidGatewayRequestException, "Cannot find resource") {
return nil, FsxFileSystemStatusNotFound, nil
} you could use something like if tfawserr.ErrCodeEquals(err, storagegateway.ErrCodeInvalidGatewayRequestException) {
var awsErr awserr.Error
if errors.As(err, &awsErr) {
nestedErr := awsErr.OrigErr()
if nestedErr != nil && tfawserr.ErrCodeEquals(nestedErr, "FileSystemAssociationNotFound") {
return nil, FsxFileSystemStatusNotFound, nil
}
}
} It's a lot more verbose, but will check against the specific error we want. It's probably something we should also add to the |
7e02ec9
to
ac93264
Compare
@gdavison thanks for the snippet. in my last 2 commits i swapped out
i also swapped out the checks in 3 other places where the previous code looks like this:
|
@gdavison OrigErr() is always nil for Storage Gateway(see attached). |
Ah, I see. The I think the approach you've taken of casting the error to a |
80529b3
to
273d950
Compare
41077c9
to
5c57e94
Compare
3c8dc8b
to
aa5c5b9
Compare
b799b57
to
905cd4d
Compare
make testacc TESTARGS='-run=TestAccAWSStorageGatewayFsxAssociateFileSystem' |
|
not sure how clear my comments back to you are after i click "resolve" but so you see it:
|
make testacc TESTARGS='-run=TestAccAWSStorageGatewayGateway' |
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's looking good. We're down to a few nitpicky details. The resource and test files will need to be renamed as well, since they're causing a linter to fail.
aws/resource_aws_storagegateway_fsx_associate_file_system_test.go
Outdated
Show resolved
Hide resolved
good catch on the file names! sorry about that! changes are pushed, running tests again then hopefully we're good? 😅 |
|
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.
Looks good! 🚀
I've made a couple tweaks to log messages and function names
Acceptance test results
--- SKIP: TestAccAWSStorageGatewayFileSystemAssociation_disappears_fsxFileSystem (0.00s)
--- SKIP: TestAccAWSStorageGatewayCachedIscsiVolume_SourceVolumeArn (0.00s)
--- PASS: TestAccAWSStorageGatewayGateway_GatewayType_FileS3 (254.46s)
--- PASS: TestAccAWSStorageGatewayCachedIscsiVolume_kms (255.46s)
--- PASS: TestAccAWSStorageGatewayGateway_GatewayType_Cached (259.00s)
--- PASS: TestAccAWSStorageGatewayCachedIscsiVolume_basic (261.17s)
--- PASS: TestAccAWSStorageGatewayGateway_GatewayType_FileFsxSmb (264.33s)
--- PASS: TestAccAWSStorageGatewayGateway_GatewayType_Vtl (264.84s)
--- PASS: TestAccAWSStorageGatewayGateway_GatewayType_Stored (276.46s)
--- PASS: TestAccAWSStorageGatewayCachedIscsiVolume_Tags (312.54s)
--- PASS: TestAccAWSStorageGatewayCache_FileGateway (314.24s)
--- PASS: TestAccAWSStorageGatewayGateway_tags (320.93s)
--- PASS: TestAccAWSStorageGatewayGateway_GatewayName (327.29s)
--- PASS: TestAccAWSStorageGatewayCache_TapeAndVolumeGateway (369.48s)
--- PASS: TestAccAWSStorageGatewayCachedIscsiVolume_SnapshotId (378.16s)
--- PASS: TestAccAWSStorageGatewayCachedIscsiVolume_disappears (381.42s)
--- PASS: TestAccAWSStorageGatewayGateway_GatewayTimezone (223.71s)
--- PASS: TestAccAWSStorageGatewayGateway_CloudWatchLogs (234.43s)
--- PASS: TestAccAWSStorageGatewayGateway_SmbGuestPassword (225.98s)
--- PASS: TestAccAWSStorageGatewayGateway_disappears (232.39s)
--- PASS: TestAccAWSStorageGatewayGateway_SMBVisibility (263.25s)
--- PASS: TestAccAWSStorageGatewayGateway_GatewayVpcEndpoint (329.29s)
--- PASS: TestAccAWSStorageGatewayGateway_SMBSecurityStrategy (275.87s)
--- PASS: TestAccAWSStorageGatewayGateway_bandwidthUpload (233.43s)
--- PASS: TestAccAWSStorageGatewayGateway_bandwidthDownload (230.60s)
--- PASS: TestAccAWSStorageGatewayGateway_bandwidthAll (229.99s)
--- PASS: TestAccAWSStorageGatewaySmbFileShare_Authentication_GuestAccess (283.46s)
--- PASS: TestAccAWSStorageGatewaySmbFileShare_Tags (274.24s)
--- PASS: TestAccAWSStorageGatewaySmbFileShare_FileShareName (282.89s)
--- PASS: TestAccAWSStorageGatewaySmbFileShare_notificationPolicy (354.39s)
--- PASS: TestAccAWSStorageGatewaySmbFileShare_DefaultStorageClass (334.89s)
--- PASS: TestAccAWSStorageGatewaySmbFileShare_accessBasedEnumeration (382.83s)
--- PASS: TestAccAWSStorageGatewaySmbFileShare_GuessMIMETypeEnabled (327.23s)
--- PASS: TestAccAWSStorageGatewaySmbFileShare_OpLocksEnabled (327.01s)
--- PASS: TestAccAWSStorageGatewaySmbFileShare_KMSEncrypted (226.58s)
--- PASS: TestAccAWSStorageGatewayGateway_SmbActiveDirectorySettings_timeout (889.82s)
--- PASS: TestAccAWSStorageGatewaySmbFileShare_ObjectACL (285.87s)
--- PASS: TestAccAWSStorageGatewaySmbFileShare_ReadOnly (284.55s)
--- PASS: TestAccAWSStorageGatewayGateway_SmbActiveDirectorySettings (956.27s)
--- PASS: TestAccAWSStorageGatewaySmbFileShare_RequesterPays (323.90s)
--- PASS: TestAccAWSStorageGatewaySmbFileShare_KMSKeyArn (389.15s)
--- PASS: TestAccAWSStorageGatewaySmbFileShare_audit (335.20s)
--- PASS: TestAccAWSStorageGatewaySmbFileShare_Authentication_ActiveDirectory (896.18s)
--- PASS: TestAccAWSStorageGatewaySmbFileShare_cacheAttributes (382.51s)
--- PASS: TestAccAWSStorageGatewaySmbFileShare_disappears (227.09s)
--- PASS: TestAccAWSStorageGatewaySmbFileShare_caseSensitivity (384.64s)
--- PASS: TestAccAWSStorageGatewaySmbFileShare_InvalidUserList (1159.15s)
--- PASS: TestAccAWSStorageGatewaySmbFileShare_ValidUserList (1036.29s)
--- PASS: TestAccAWSStorageGatewaySmbFileShare_smb_acl (1056.29s)
--- PASS: TestAccAWSStorageGatewayGateway_SmbMicrosoftActiveDirectorySettings_timeout (1916.68s)
--- PASS: TestAccAWSStorageGatewaySmbFileShare_AdminUserList (1036.00s)
--- PASS: TestAccAWSStorageGatewayGateway_SmbMicrosoftActiveDirectorySettings (2110.29s)
--- PASS: TestAccAWSStorageGatewayFileSystemAssociation_disappears_storageGateway (3397.29s)
--- PASS: TestAccAWSStorageGatewayFileSystemAssociation_tags (3484.10s)
--- PASS: TestAccAWSStorageGatewayFileSystemAssociation_disappears (3723.21s)
--- PASS: TestAccAWSStorageGatewayFileSystemAssociation_auditDestination (3768.46s)
--- PASS: TestAccAWSStorageGatewayFileSystemAssociation_cacheAttributes (4042.87s)
--- PASS: TestAccAWSStorageGatewayFileSystemAssociation_basic (3472.83s)
This functionality has been released in v3.52.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. Thank you! |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. |
Community Note
Closes #19612
Output from acceptance testing:
Big thanks to @AdamTylerLynch who wrote just as much of this as i did
also thanks to @gdavison, @ewbankkit, and @DrFaust92