search

StackExchange / dnscontrol

Infrastructure as code for DNS!
https://dnscontrol.org/
MIT License
3.08k stars 390 forks source link

DIFF2: Ongoing discussion #1854

Closed tlimoncelli closed 1 year ago

tlimoncelli commented 1 year ago
vatsalyagoel commented 1 year ago

You mentioned that AzureDNS provider is buggy with Diff2, can you expand a bit on that. I have some time this week that I can look to resolve the bugs.

tlimoncelli commented 1 year ago

vatsalyagoel Thanks for asking!

The existing code is my first attempt. It mostly works but ... not entirely. Sadly I don't know the Azure APIs well enough to determine what I've done wrong. Would you mind taking a look?

Here's a repo case:

cd $git/dnscontrol/integrationTest
go test -v -verbose -provider AZURE_DNS -diff2 -end 1

Tom

masterzen commented 1 year ago

@tlimoncelli please see #1861 for adaptation of the OVH provider to diff2. Tests are passing in both modes.

tlimoncelli commented 1 year ago

Hey folks! I have good news! I've made it a lot easier to adopt the diff2 code. Since the diff2 code makes it easy to have one algorithm that packages the results different ways, I realized I could package the results exactly as the old pkg/diff code presents it. Yes, that means extra work is being done behind the scenes, but its just moving pointers around so it won't be too much of a performance burden.

To adopt this method, simply call diff.NewCompat(dc) instead of diff.New(dc). Here's an example:

  var corrections []*models.Correction
  var create, del, mod diff.Changeset
  if !diff2.EnableDiff2 {
    differ := diff.New(dc)
    _, create, del, mod, err = differ.IncrementalDiff(actual)
  } else {
    differ := diff.NewCompat(dc)
    _, create, del, mod, err = differ.IncrementalDiff(actual)
  }
  if err != nil {
    return nil, err
  }

I would prefer that people adopt the new pkg/diff2/By*() functions eventually, but this new method has the benefit of getting people off the old code faster.

Once everyone is off the old code, I will introduce the new IGNORE*() features which are much more reliable in the pkg/diff2 world.

systemcrash commented 1 year ago

Hey folks! I have good news! I've made it a lot easier to adopt the diff2 code. Since the diff2 code makes it easy to have one algorithm that packages the results different ways, I realized I could package the results exactly as the old pkg/diff code presents it. Yes, that means extra work is being done behind the scenes, but its just moving pointers around so it won't be too much of a performance burden.

To adopt this method, simply call diff.NewCompat(dc) instead of diff.New(dc). Here's an example:

  var corrections []*models.Correction
  var create, del, mod diff.Changeset
  if !diff2.EnableDiff2 {
    differ := diff.New(dc)
    _, create, del, mod, err = differ.IncrementalDiff(actual)
  } else {
    differ := diff.NewCompat(dc)
    _, create, del, mod, err = differ.IncrementalDiff(actual)
  }
  if err != nil {
    return nil, err
  }

I would prefer that people adopt the new pkg/diff2/By*() functions eventually, but this new method has the benefit of getting people off the old code faster.

Once everyone is off the old code, I will introduce the new IGNORE*() features which are much more reliable in the pkg/diff2 world.

Thought it worthwhile to chime in here. Your code-block is nice and short and succinct, but... the err variable passes immediately out of scope and the error check outside of the if block does not happen.

I noticed this after I ran staticcheck ./... so to pass the check I had to:

    if !diff2.EnableDiff2 {
        differ := diff.New(dc)
        _, create, del, modify, err = differ.IncrementalDiff(existingRecords)
        if err != nil {
            return nil, err
        }
    } else {
        differ := diff.NewCompat(dc)
        _, create, del, modify, err = differ.IncrementalDiff(existingRecords)
        if err != nil {
            return nil, err
        }
    }

declaring err before the if block is also an option.

tlimoncelli commented 1 year ago

declaring err before the if block is also an option.

Good catch! I'm surprised that staticcheck didn't catch that. It catches similar problems.

I see that providers/packetframe/packetframeProvider.go fixed this a different way. I'll review both and make the change.

tlimoncelli commented 1 year ago

FYI: The "err" issue is split out to https://github.com/StackExchange/dnscontrol/issues/2159

systemcrash commented 1 year ago

I might actually be in error here. (no pun) Other providers seem to correctly declare err before the if block - although the staticcheck picked up on this for me because I did not have err declared before the block. Anyway, if we catch some more potential errors, even better. Sorry for the noise.

tlimoncelli commented 1 year ago

@https://github.com/arnoschoon we haven't heard from you. I'm going to close this issue and ask that we pick it up in https://github.com/StackExchange/dnscontrol/issues/2214