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

Fix parsing of digitalocean dns records #14215

Merged

Conversation

mjsteger
Copy link
Contributor

@mjsteger mjsteger commented May 4, 2017

Currently if one uses a file like:


# Configure the DigitalOcean Provider
provider "digitalocean" {
  token = "${var.do_token}"
}

resource "digitalocean_record" "mx" {
  name = "foobaz"
  domain = "thistesting1.com"
  type = "MX"
  value = "thistesting1.com."
  priority = "10"
}

Then terraform will constantly suggest that the changes are necessary, as the parsing of these records is currently incorrect.

This changeset fixes how some digitalocean dns records were getting parsed. In particular, it allows for understanding "@" as shorthand for the domain itself, preventing terraform from suggesting changes that wouldn't have any actual effect. This changeset also adds a trailing "." to certain record types which are required to be submitted with a trailing dot, but which digitalocean does not return with a trailing dot, again preventing changes that wouldn't have an effect.

Tests have been added for the above, and with just adding the tests, the current code is failing, as it is handling some records(e.g. MX) incorrectly.

Let me know if you have any questions!

This changeset fixes how some digitalocean dns records were getting
parsed. In particular, it allows for understanding "@" as shorthand for
the domain itself, preventing terraform from suggesting changes that
wouldn't have any actual effect. This changeset also adds a trailing "."
to certain record types which are required to be submitted with a
trailing dot, but which digitalocean does not return with a trailing
dot, again preventing changes that wouldn't have an effect.

Tests have been added for the above, and with just adding the tests, the
current code is failing, as it is handling some records(e.g. MX)
incorrectly
@mjsteger
Copy link
Contributor Author

mjsteger commented May 9, 2017

Is there anything I can do to help get this merged in? @radeksimko

@stack72
Copy link
Contributor

stack72 commented May 16, 2017

This LGTM! Thanks forthis :)

% make testacc TEST=./builtin/providers/digitalocean TESTARGS='-run=TestAccDigitalOceanRecord'     ✭
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/05/16 12:07:25 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/digitalocean -v -run=TestAccDigitalOceanRecord -timeout 120m
=== RUN   TestAccDigitalOceanRecord_Basic
--- PASS: TestAccDigitalOceanRecord_Basic (9.49s)
=== RUN   TestAccDigitalOceanRecord_Updated
--- PASS: TestAccDigitalOceanRecord_Updated (22.33s)
=== RUN   TestAccDigitalOceanRecord_HostnameValue
--- PASS: TestAccDigitalOceanRecord_HostnameValue (6.85s)
=== RUN   TestAccDigitalOceanRecord_ExternalHostnameValue
--- PASS: TestAccDigitalOceanRecord_ExternalHostnameValue (14.68s)
=== RUN   TestAccDigitalOceanRecord_MX
--- PASS: TestAccDigitalOceanRecord_MX (7.17s)
=== RUN   TestAccDigitalOceanRecord_MX_at
--- PASS: TestAccDigitalOceanRecord_MX_at (12.08s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/digitalocean	72.610s

@stack72 stack72 merged commit 9c0888c into hashicorp:master May 16, 2017
@ghost
Copy link

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