8000 updating a DNS record does not work as expected · Issue #3 · soniah/dnsmadeeasy · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

updating a DNS record does not work as expected #3

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.

Sign up for GitHub

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

Jump to bottom
Open
JeanMertz opened this issue Jul 24, 2015 · 4 comments
Open

updating a DNS record does not work as expected #3

JeanMertz opened this issue Jul 24, 2015 · 4 comments

Comments

@JeanMertz
Copy link

see: hashicorp/terraform#2241
@samcrang
Copy link

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
Copy link
Owner
soniah commented Nov 15, 2015

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
Copy link

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
Copy link
Owner
soniah commented Nov 16, 2015

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.

@samcrang samcrang mentioned this issue Nov 18, 2015
Closed
@benosman benosman mentioned this issue Sep 24, 2016
Closed
@hashibot hashibot mentioned this issue Jun 13, 2017
Closed
@emsearcy emsearcy mentioned this issue Dec 1, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@soniah @samcrang @JeanMertz
0