Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Linode provider #268

Merged
merged 5 commits into from
Nov 15, 2017
Merged

Linode provider #268

merged 5 commits into from
Nov 15, 2017

Conversation

koesie10
Copy link
Contributor

Fixes #121

All tests pass, except for 25 because Linode performs subdomain validation which does not allow the 企业 TLD.

@pmoroney
Copy link
Contributor

By subdomain validation, do you mean they check to see if the ööö.企业. record exists?

@koesie10
Copy link
Contributor Author

I'm sorry, I meant TLD validation, so the 企业 TLD is not in their systems and thus not allowed to be added as a target for a CNAME record.

@pmoroney
Copy link
Contributor

I reached out to Linode about the TLD.
Also I think the checks for the PR are failing because you need to run go generate and then commit those changes too.

Original: &domainRecord{},
})
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add:

  // Normalize
  models.Downcase(foundRecords)

MxPreference: uint16(r.Priority),
SrvPriority: uint16(r.Priority),
SrvWeight: uint16(r.Weight),
SrvPort: uint16(r.Port),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could domainRecord use uint32/uint16 instead of int? Then the conversions wouldn't be needed here.

req.Name = ""
case "CNAME":
req.Target = fixTarget(req.Target, dc.Name)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switch statements should list all possible rtypes so that when a new rtype is added, we fail fast. Look at https://github.com/StackExchange/dnscontrol/blob/master/models/dns.go#L90 for an example.

}
}

func fixTTL(ttl uint32) uint32 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the code modifies TTLs, the docs should make note of that.

"testing"
)

func TestFixTTL(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a unit test for {299, ???} (i.e. that way you are testing, n, n-1, and n+1)

(by the way... thank you so much for having unit tests. So few people write them and they are very important for keeping the code maintainable)

@koesie10 koesie10 force-pushed the linode branch 2 times, most recently from 74c9adf to b716bd5 Compare November 14, 2017 22:22
@tlimoncelli
Copy link
Contributor

Thanks for the contribution!

@tlimoncelli tlimoncelli merged commit 9a44e78 into StackExchange:master Nov 15, 2017
@koesie10 koesie10 deleted the linode branch November 15, 2017 12:03
pmoroney pushed a commit to pmoroney/dnscontrol that referenced this pull request Jan 11, 2018
rblenkinsopp pushed a commit to rblenkinsopp/dnscontrol that referenced this pull request Aug 21, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants