-
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
Service details not yet available in GovCloud. #3514
Conversation
Revert to logic from 1.8 for GovCloud until service details are supported.
I can knock out a test if someone wouldn't mind providing a windows binary... though I can probably compile one myself tomorrow. |
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.
I think we can skip the isGovCloud
logic to handle both API responses in this case. The added bonus will also be that US Government's data source response will be enhanced when they upgrade the API in that partition.
e.g.
// replacing existing if resp == nil || len(resp.ServiceDetails) == 0 {
if resp == nil || len(resp.ServiceNames) == 0 {
return fmt.Errorf("no matching VPC Endpoint Service found")
}
if len(resp.ServiceNames) > 1 {
return fmt.Errorf("multiple VPC Endpoint Services matched; use additional constraints to reduce matches to a single VPC Endpoint Service")
}
if len(resp.ServiceDetails) == 0 {
d.SetId(strconv.Itoa(hashcode.String(*resp.ServiceNames[0])))
d.Set("service_name", resp.ServiceNames[0])
return nil
}
// continue with sd := resp.ServiceDetails[0] logic onward
|
||
// Note: AWS Commercial now returns a response with `ServiceNames` and | ||
// `ServiceDetails`, but GovCloud responses only include `ServiceNames` | ||
if isGovCloud && resp.ServiceDetails == nil { |
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.
As written, this line can cause a panic if resp == nil
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.
LGTM, thanks! 🚀
Standard:
make testacc TEST=./aws TESTARGS='-run=TestAccDataSourceAwsVpcEndpointService'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccDataSourceAwsVpcEndpointService -timeout 120m
=== RUN TestAccDataSourceAwsVpcEndpointService_gateway
--- PASS: TestAccDataSourceAwsVpcEndpointService_gateway (10.59s)
=== RUN TestAccDataSourceAwsVpcEndpointService_interface
--- PASS: TestAccDataSourceAwsVpcEndpointService_interface (9.43s)
=== RUN TestAccDataSourceAwsVpcEndpointService_custom
--- PASS: TestAccDataSourceAwsVpcEndpointService_custom (298.66s)
PASS
ok github.com/terraform-providers/terraform-provider-aws/aws 318.713s
US Gov (manually):
terraform apply
data.aws_vpc_endpoint_service.s3: Refreshing state...
Apply complete! Resources: 0 added, 0 changed, 0 destroyed.
Outputs:
s3_endpoint = com.amazonaws.us-gov-west-1.s3
This has been released in version 1.11.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. |
@bflad Thanks, I just tested it and it looks like just going from v1.8.0 => v1.11.0 and changing nothing else at all it wants to destroy and recreate the s3 endpoint. |
Ahh, looks like this is the line that's changing and causing the new resource:
|
Sorry for any confusion, @lorengordon, this pull request was specifically for the |
@bflad No worries, thanks for following up. I just subscribed to that PR also! |
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! |
Revert to logic from 1.8 for GovCloud until service details are
supported.
Resolves #3506.
Note: I haven't had time to test this thoroughly, so it would be great to get help testing this out. cc @lorengordon @bflad