updating a DNS record does not work as expected

[JeanMertz]

see: hashicorp/terraform#2241

[samcrang]

I've just spent time looking at this. There appears to be a couple of things going on:

1. When this was originally written it relied on apparently broken behavior in mergo.Map. The behavior this provider wants has been added as mergo.MapWithOverwrite.
2. There are some issues with field casing. The behavior of mergo.MapWithOverwrite when the destination is a struct and the source is map[string]interface{} is to attempt to capitalize fields in the source and match them against fields on the struct. For the most part this is fine with fields such as Value, however, Terraform passes ttl through in lowercase and thus Mergo fails to successfully do a merge onto a struct with field TTL.

I had a go at fixing the first problem (tests are green but the implementation isn't correct) before uncovering the second one (run the tests and you'll see the failure).

In this case I kind of feel like Mergo complicates/confuses things and I'd be tempted to remove it entirely, but I don't want to make any opinionated changes without checking if this seems sensible first.

@soniah have you got any thoughts on this?

[soniah]

Hi Sam @samcrang

Thanks for your very detailed bug report - I wish all bug reports were like this! I hadn't noticed outstanding issues on this repo - my father died in May :-( and I've been rather busy since then dealing with his estate.

I've created branch mergo_case_issue in my repo with a possible fix -- what do you think?

I haven't thought fully about removing mergo yet (and it's Monday morning at work). It's always good to remove libraries - could this merging be done easily in my code? Or were you thinking of using a different library?

cc: @JeanMertz

[samcrang]

I'm sorry to hear about father--I understand entirely if working on this repo isn't something that's a priority for you right now. I'm happy to take some time looking at getting fixes for the issues.

I had just thought about re-implementing the specific parts of Mergo that this project uses/cares about (with some additional magic to deal with the case-insensitivity), however, your fix is probably the most pragmatic thing to do right now. I'd like to add a bit more test coverage around the merging though--I'm happy to do this and submit a pull request.

Also I was hoping to refactor the tests so we can remove the custom test HTTP server and replace it with net/http/httptest--would that be okay with you?

This would all be with the ultimate aim to get more coverage of the DNSMadeEasy API so we can ultimately start Terraforming all the other good stuff that DME provide. Again I'm happy to work on this and submit PRs. Does this seem reasonable? I don't want to dump a whole load of changes into your project with checking it seems sensible first! :)

Thanks,

[soniah]

Hi Sam,

Thanks for all your work 😀 I think go for better test coverage first, then
I can submit the changes to the mainline of Terraform, since it's breaking
things. Then have a go at the mergo and net/http/httptest replacements.

The other issue - I haven't looked at it yet. But you're welcome to have a
crack it, if/when you have time... And of course if you add other DME
features that would be fantastic 👍

Sonia.

On 17 November 2015 at 03:51, Sam Crang notifications@github.com wrote:

I'm sorry to hear about father--I understand entirely if working on this
repo isn't something that's a priority for you right now. I'm happy to take
some time looking at getting fixes for the issues.

I had just thought about re-implementing the specific parts of Mergo that
this project uses/cares about (with some additional magic to deal with the
case-insensitivity), however, your fix is probably the most pragmatic thing
to do right now. I'd like to add a bit more test coverage around the
merging though--I'm happy to do this and submit a pull request.

Also I was hoping to refactor the tests so we can remove the custom test
HTTP server and replace it with net/http/httptest--would that be okay with
you?

This would all be with the ultimate aim to get more coverage of the
DNSMadeEasy API so we can ultimately start Terraforming all the other good
stuff that DME provide. Again I'm happy to work on this and submit PRs.
Does this seem reasonable? I don't want to dump a whole load of changes
into your project with checking it seems sensible first! :)

Thanks,

—
Reply to this email directly or view it on GitHub.