Fix failing tests

[mbag]

The tests are failing after merging #74 the reason was identified, I'm just waiting for confirmation in this thread: #74 (comment)

If this simple fix would not fix the issue, I will revert commit until issue with tests and import is resolved.

[jbe-dw]

Sorry at first it was a simple proposal, and i enhanced the code afterwards too quickly.

[mbag]

No problem, it was also my mistake for not running tests and accepting the PR too quickly.

[jbe-dw]

@mbag I closed my patch after another set of tests. I'm not figuring out any solution with terraform SDK

This SDK is fu**in twisted and we can not say that not putting a nameserver get no changes. ATM the only 'viable' solution is to remove this option and let NS be managed by powerdns_records. I've tried to turn all the options arround and we are not even able to say that the default is an empty list.

Thanks hashicorp !

[jbe-dw]

@mbag I've been busy on other topics lately, but as I said I tried every option Terraform SDK offers and could not figure something that is working out.

The best way would be to revert this last patch. But my conclusion is still that we may not offer NS record management through the zone resource (even if the API allow to set it up at creation) and let the users manage them via the record resource.

[jbe-dw]

@mbag Hello,

It has been weeks since we discussed about this. You should definitely revert back the last PR. What is your thoughts about removing NS support from the zone module ? It would be a breaking change.

[selfuryon]

Give me several days - I will fix it with PR for #60

[selfuryon]

Oh, I got the issue, and I agree about that:

This SDK is fu**in twisted and we can not say that not putting a nameserver get no changes. ATM the only 'viable' solution is to remove this option and let NS be managed by powerdns_records. I've tried to turn all the options arround and we are not even able to say that the default is an empty list.

And removing NS support from a zone is a good solution maybe, it would be the same logic as other RR management

[mbag]

Hi sorry for not responding for a while.

What is your thoughts about removing NS support from the zone module ? It would be a breaking change.

I'm not against having NS zone update, I think it's good and needed feature. Feel free to reopen pull request again, but import part needs to be solved somehow, before we can use it.

[mbag]

I'm closing this issue, but the discussion can continue either here, or on some new issue.

[jbe-dw]

Hello,

I'm not into the code for a while, so I'm not 100% sure about what i'm about to say but we can't delegate NS record management to the record module for now. The zone module would change them if they do not match the record configurations. Not declaring them at all in the zone make them destroyed if I remember correctly.

For me managing a zone should also manage, at least, the SOA and NS records, but it's not tied at all in PDNS and the record module is doing the job. Then some would say that you also need to manage MX or some TXT records too. But it would be easier to managing them with the record module, and not let the zone module interfere. Supporting them would require a lot of changes with no extra value.

Removing it would cause changes to users not aware of it when upgrading. If you are not agreeing to this, I won't spend extra time in a PR and stay on my private fork.

Can you give me an insight about the import part, I forgot and not sure about what you mean.

Thank you

[jbe-dw]

@mbag you can close it I guess