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: Add ability to update r53 zone comment #5318

Merged
merged 1 commit into from
Feb 26, 2016

Conversation

stack72
Copy link
Contributor

@stack72 stack72 commented Feb 25, 2016

As per request in #5317

make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSRoute53Zone_'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /vendor/)
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSRoute53Zone -timeout 120m
=== RUN   TestAccAWSRoute53Zone_basic
--- PASS: TestAccAWSRoute53Zone_basic (43.16s)
=== RUN   TestAccAWSRoute53Zone_updateComment
--- PASS: TestAccAWSRoute53Zone_updateComment (49.10s)
=== RUN   TestAccAWSRoute53Zone_private_basic
--- PASS: TestAccAWSRoute53Zone_private_basic (64.86s)
=== RUN   TestAccAWSRoute53Zone_private_region
--- PASS: TestAccAWSRoute53Zone_private_region (64.09s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    231.942s

@@ -197,6 +201,19 @@ func resourceAwsRoute53ZoneRead(d *schema.ResourceData, meta interface{}) error
func resourceAwsRoute53ZoneUpdate(d *schema.ResourceData, meta interface{}) error {
conn := meta.(*AWSClient).r53conn

if d.HasChange("comment") {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't comment be set as partial + d.Partial(true) used? It's one API call (out of two) that can fail, right?
Before it was only 1 call for tags, so I don't think d.SetPartial("tags") was actually necessary before.

btw. I think d.SetPartial("tags") in the place where it is now (else block) is completely useless (putting away the fact that d.Partial(true) isn't called), but maybe I'm just misunderstanding the way this works?

@radeksimko
Copy link
Member

Acceptance tests pass, this looks ok except the partial state handling.

@radeksimko radeksimko added the waiting-response An issue/pull request is waiting for a response from the community label Feb 25, 2016
@stack72 stack72 force-pushed the f-aws-route53zone-updateComment branch from 52f4cac to d750d4d Compare February 26, 2016 14:42
@stack72
Copy link
Contributor Author

stack72 commented Feb 26, 2016

@radeksimko have another quick look at this when you get a chance and let me know if you feel it's ready :)

@radeksimko radeksimko removed the waiting-response An issue/pull request is waiting for a response from the community label Feb 26, 2016
@jen20
Copy link
Contributor

jen20 commented Feb 26, 2016

LGTM.

stack72 added a commit that referenced this pull request Feb 26, 2016
provider/aws: Add ability to update r53 zone comment
@stack72 stack72 merged commit a372800 into hashicorp:master Feb 26, 2016
@stack72 stack72 deleted the f-aws-route53zone-updateComment branch February 26, 2016 19:43
@ghost
Copy link

ghost commented Apr 27, 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 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants