Skip to content
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

provider/aws: FIxed the api_gw_domain_name replace operation #10179

Merged
merged 1 commit into from
Dec 7, 2016

Conversation

Ninir
Copy link
Contributor

@Ninir Ninir commented Nov 16, 2016

This fixes partially #8789.

When you try to update the certificate_name, which is allowed per https://docs.aws.amazon.com/apigateway/api-reference/link-relation/domainname-update/, you get the following response:

aws_api_gateway_domain_name.hsts: BadRequestException: Invalid patch path '/certificate_name' specified for op 'replace'. Must be one of: [/certificateName]

This updates the replace paths in order to remove this error. This does not try to fix the issue about updating the certificate, which should be about forcing a new resource.

Another PR will follow which will be about forcing new resources for certificate changes.

@stack72
Copy link
Contributor

stack72 commented Dec 7, 2016

Hi @Ninir

Doesn't look like the tests work as expected here -

% make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSAPIGatewayDomainName_'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/12/07 13:43:29 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSAPIGatewayDomainName_ -timeout 120m
=== RUN   TestAccAWSAPIGatewayDomainName_basic
--- FAIL: TestAccAWSAPIGatewayDomainName_basic (0.00s)
	testing.go:265: Step 0 error: Error loading configuration: Error parsing /var/folders/gd/yv86v7pd1cd5dpzjrw8702k00000gn/T/tf-test603301176/main.tf: At 15:59: expected '/' for comment
FAIL
exit status 1
FAIL	github.com/hashicorp/terraform/builtin/providers/aws	0.025s
make: *** [testacc] Error 1

Could you take a look?

Thanks

Paul

@stack72 stack72 added the waiting-response An issue/pull request is waiting for a response from the community label Dec 7, 2016
@Ninir
Copy link
Contributor Author

Ninir commented Dec 7, 2016

Hi @stack72

It is the same on the master branch apparently, will investigate and fix it in this one too :)
Thanks for spotting this!

@Ninir Ninir force-pushed the api_gw_replace_operations branch from 5d424e8 to 56c35b9 Compare December 7, 2016 16:42
@Ninir
Copy link
Contributor Author

Ninir commented Dec 7, 2016

Found the issue, it was about the way to set the certificate data, which was wrong since the beginning unfortunately :/. ('Think I forgot to ran tests when throwing the PR... should be all good now :))

Here are the acceptance tests:

$ make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSAPIGatewayDomainName_'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/12/07 17:40:00 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSAPIGatewayDomainName_ -timeout 120m
=== RUN   TestAccAWSAPIGatewayDomainName_basic
--- PASS: TestAccAWSAPIGatewayDomainName_basic (70.32s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/aws	70.349s

certificate_body = "%v"
certificate_chain = "%v"
certificate_body = <<EOF
%vEOF
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is done on purpose to avoid a diff issue. The other solution would be to update line 43 for instance, adding:

...
resource.TestCheckResourceAttr("aws_api_gateway_domain_name.test", "certificate_body", fmt.Sprintf("%v\n", testAccAWSAPIGatewayCertBody),
...

@stack72
Copy link
Contributor

stack72 commented Dec 7, 2016

Thanks for the work herre @Ninir :)

% make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSAPIGatewayDomainName_'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/12/07 21:27:36 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSAPIGatewayDomainName_ -timeout 120m
=== RUN   TestAccAWSAPIGatewayDomainName_basic
--- PASS: TestAccAWSAPIGatewayDomainName_basic (54.79s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/aws	54.824s

@stack72 stack72 merged commit 4b25837 into hashicorp:master Dec 7, 2016
@Ninir Ninir deleted the api_gw_replace_operations branch December 7, 2016 21:32
@Ninir
Copy link
Contributor Author

Ninir commented Dec 7, 2016

Thank you for the review @stack72 :)

ojongerius added a commit to atlassian/terraform that referenced this pull request Dec 8, 2016
…dentials

* upstream/master: (79 commits)
  update CHANGELOG
  Update panicwrap to pass through all interrupt signals
  Gracefully stops on SIGTERM
  website: update website for conditionals
  vendor: update HIL with conditionals
  Keep a consistent provider order.
  Update CHANGELOG.md
  provider/aws: Forces the api gateway domain name certificates to recreate the resource (hashicorp#10588)
  Update CHANGELOG.md
  provider/aws: FIxed the api_gw_domain_name replace operation (hashicorp#10179)
  Fixed note formatting
  Explicitly say `count` is not supported by modules (hashicorp#10553)
  docs/aws: Fix the discrepencies of the emr_cluster documentation (hashicorp#10578)
  Update CHANGELOG.md
  Service role is not updated on AWS for a CodeDeploy deployment group (hashicorp#9866)
  Update CHANGELOG.md
  provider/datadog hashicorp#9375: Refactor tags to a list instead of a map. (hashicorp#10570)
  Update the Vagrantfile to resolve package update/installation issue. (hashicorp#9783)
  docs/aws: Add iam_server_certificate data source to nav bar (hashicorp#10576)
  Update CHANGELOG.md
  ...
pielu pushed a commit to pielu/terraform that referenced this pull request Dec 20, 2016
* aws/feature/r-instance-net-iface-id: (74 commits)
  - Properly exercise network_interface_id from AWS SDK - Update Terraform’s documentation
  Update CHANGELOG.md
  provider/aws: Forces the api gateway domain name certificates to recreate the resource (hashicorp#10588)
  Update CHANGELOG.md
  provider/aws: FIxed the api_gw_domain_name replace operation (hashicorp#10179)
  Fixed note formatting
  Explicitly say `count` is not supported by modules (hashicorp#10553)
  docs/aws: Fix the discrepencies of the emr_cluster documentation (hashicorp#10578)
  Update CHANGELOG.md
  Service role is not updated on AWS for a CodeDeploy deployment group (hashicorp#9866)
  Update CHANGELOG.md
  provider/datadog hashicorp#9375: Refactor tags to a list instead of a map. (hashicorp#10570)
  Update the Vagrantfile to resolve package update/installation issue. (hashicorp#9783)
  docs/aws: Add iam_server_certificate data source to nav bar (hashicorp#10576)
  Update CHANGELOG.md
  feat/aws: add iam_server_certificate data source (hashicorp#10558)
  provider/azurerm: arm_virtual_machine panic fix
  Update .travis.yml
  provider/aws: Improved the documentation for EMR Cluster (hashicorp#10563)
  provider/azurerm: Do not pass an empty string of license_type to AMR VMs (hashicorp#10564)
  ...

# Conflicts:
#	builtin/providers/aws/resource_aws_instance.go
@ghost
Copy link

ghost commented Apr 19, 2020

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug provider/aws waiting-response An issue/pull request is waiting for a response from the community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants