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

Importability for Google DNS Managed Zone #13824

Merged

Conversation

JDiPierro
Copy link
Contributor

Requested in #13416.

I removed the default description for the DNS managed zone because when I imported a manually created zone with no description it wanted to recreate it with the default terraform description. Since Description is an optional field I think it's appropriate to make it empty by default.

$ make test TEST=./builtin/providers/google/
==> Checking that code complies with gofmt requirements...
==> Checking AWS provider for unchecked errors...
==> NOTE: at this time we only look for uncheck errors in the AWS package
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/04/20 17:10:57 Generated command/internal_plugin_list.go
go test -i ./builtin/providers/google/ || exit 1
echo ./builtin/providers/google/ | \
		xargs -t -n4 go test  -timeout=60s -parallel=4
go test -timeout=60s -parallel=4 ./builtin/providers/google/
ok  	github.com/hashicorp/terraform/builtin/providers/google	0.027s

@@ -32,7 +34,6 @@ func resourceDnsManagedZone() *schema.Resource {
Type: schema.TypeString,
Optional: true,
ForceNew: true,
Default: "Managed by Terraform",
Copy link
Contributor

Choose a reason for hiding this comment

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

My only concern about this is that removing the default would lead Terraform to think every managed zone changed, and try to destroy then re-create it. I'm not sure what the best way to get rid of a default value is (or support imports when dealing with a default value) without causing too much backwards incompatibility. I'll check with the team and see if anyone has advice and report back.

Copy link
Contributor

Choose a reason for hiding this comment

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

So I think if you leave the Default as it is and set this to Computed: true, things should be OK and not cause problems. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds great, I'll get that in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eek!
resource google_dns_managed_zone: description: Default must be nil if computed
I'll leave the default as-is. Users will have to manually provide an empty description on imported managed zones.

@paddycarver paddycarver self-requested a review April 25, 2017 00:01
@JDiPierro JDiPierro force-pushed the import_google_dns_managed_zone branch from 9627d10 to d80885f Compare May 2, 2017 00:28
@JDiPierro
Copy link
Contributor Author

Re-added the default description.

==> Checking that code complies with gofmt requirements...
==> Checking AWS provider for unchecked errors...
==> NOTE: at this time we only look for uncheck errors in the AWS package
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/05/01 20:27:58 Generated command/internal_plugin_list.go
go test -i ./builtin/providers/google/ || exit 1
echo ./builtin/providers/google/ | \
		xargs -t -n4 go test  -timeout=60s -parallel=4
go test -timeout=60s -parallel=4 ./builtin/providers/google/
ok  	github.com/hashicorp/terraform/builtin/providers/google	0.037s```

Copy link
Contributor

@paddycarver paddycarver left a comment

Choose a reason for hiding this comment

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

LGTM, merging! Great job, thanks so much for contributing this functionality. I notice you've been on a sweep; you're doing great work, and it's really appreciated.

@paddycarver paddycarver merged commit 7a73c21 into hashicorp:master May 3, 2017
@ghost
Copy link

ghost commented Apr 13, 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 13, 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.

2 participants