Clarify dev docs (#734)

* Clarify dev docs

* fixup!
This commit is contained in:
Tom Limoncelli 2020-05-08 10:56:58 -04:00 committed by GitHub
parent 8dd66ec605
commit d8a153c01f
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
3 changed files with 95 additions and 45 deletions

View file

@ -6,8 +6,8 @@ title: Creating new DNS Resource Types (rtypes)
# Creating new DNS Resource Types (rtypes) # Creating new DNS Resource Types (rtypes)
Everyone is familiar with A, AAAA, CNAME, NS and other Rtypes. Everyone is familiar with A, AAAA, CNAME, NS and other Rtypes.
However there are new record types being added all the time (possibly However there are new record types being added all the time.
too many). Each new record type requires special handling by Each new record type requires special handling by
DNSControl. DNSControl.
If a record simply has a single "target", then there is little to If a record simply has a single "target", then there is little to
@ -34,7 +34,7 @@ Here are some examples:
``` ```
type RecordConfig struct { type RecordConfig struct {
... ...
MxPreference uint16 `json:"mxpreference,omitempty"` // FIXME(tlim): Rename to MxPreference MxPreference uint16 `json:"mxpreference,omitempty"`
SrvPriority uint16 `json:"srvpriority,omitempty"` SrvPriority uint16 `json:"srvpriority,omitempty"`
SrvWeight uint16 `json:"srvweight,omitempty"` SrvWeight uint16 `json:"srvweight,omitempty"`
SrvPort uint16 `json:"srvport,omitempty"` SrvPort uint16 `json:"srvport,omitempty"`
@ -48,8 +48,7 @@ type RecordConfig struct {
You'll need to mark which providers support this record type. The You'll need to mark which providers support this record type. The
initial PR should implement this record for the `bind` provider at initial PR should implement this record for the `bind` provider at
a minimum, unless this is a fake or pseudo-type that only a particular a minimum.
provider supports.
* Add the capability to the file `dnscontrol/providers/capabilities.go` (look for `CanUseAlias` and add * Add the capability to the file `dnscontrol/providers/capabilities.go` (look for `CanUseAlias` and add
it to the end of the list.) it to the end of the list.)
@ -97,7 +96,15 @@ your code conforms to our coding standard:
npm install prettier npm install prettier
node_modules/.bin/prettier --write pkg/js/helpers.js node_modules/.bin/prettier --write pkg/js/helpers.js
FYI: If you change `pkg/js/helpers.js`, run `go generate` to update `pkg/js/static.go`. Any time you change `pkg/js/helpers.js`, run `go generate` to regenerate `pkg/js/static.go`.
The dnscontrol `-dev` flag ignores `pkg/js/static.go` and reads
`pkg/js/helpers.js` directly. This is useful when debugging since it
is one less step.
Likewise, if you are debugging helpers.js and you can't figure out why
your changes aren't making a difference, it usually means you aren't
running `go generate` after any change, or using the `-dev` flag.
## Step 4: Search for `#rtype_variations` ## Step 4: Search for `#rtype_variations`
@ -129,32 +136,37 @@ validation. This is explained in Step 2 (search for
## Step 6: Add an `integrationTest` test case. ## Step 6: Add an `integrationTest` test case.
Add at least one test case to the `integrationTest/integration_test.go` Add at least one test case to the `integrationTest/integration_test.go`
file. Look for `var tests =` and add the test to the end of this file. Look for `func makeTests` and add the test to the end of this
list. list.
Each entry in the list is a new state. For example: Each `testgroup()` is a named list of tests.
``` ```
// MX testgroup("MX", <<< 1
tc("Empty"), <<< 1
tc("MX record", mx("@", 5, "foo.com.")), <<< 2 tc("MX record", mx("@", 5, "foo.com.")), <<< 2
tc("Change MX pref", mx("@", 10, "foo.com.")), <<< 3 tc("Change MX pref", mx("@", 10, "foo.com.")), <<< 3
tc("MX record", <<< 4
mx("@", 10, "foo.com."),
mx("@", 20, "bar.com."),
),
)
``` ```
Line 1: An `tc()` entry with no records (just a comment). The test Line 1: `testgroup()` gives a name to a group of tests. It also tells
system will delete all records from the domain to make the domain the system to delete all records for this domain so that the tests
match this empty configuration. This creates a "clean slate" begin with a blank slate.
situation.
Line 2: A `tc()` entry with 1 record. To get to this state, the Line 2:
provider will have to add the record. If this works, basic functionality Each `tc()` encodes all the records of a zone. The test framework
for the MX record type has been achieved. will try to do the smallest changes to bring the zone up to date.
In this case, we know the zone is empty, so this will add one MX
record.
Line 3: A `tc()` entry with 1 record, with a different priority. Line 3: In this example, we just change one field of an existing
To get to this state, the provider will have to either change the record. To get to this configuration, the provider will have to
priority on an existing record, or delete the old record and insert either change the priority on an existing record, or delete the old
a new one. Either way, this test case assures us that the diff'ing record and insert a new one. Either way, this test case assures us
functionality is working properly. that the diff'ing functionality is working properly.
If you look at the tests for `CAA`, it inserts a few records then If you look at the tests for `CAA`, it inserts a few records then
attempts to modify each field of a record one at a time. This test attempts to modify each field of a record one at a time. This test
@ -162,10 +174,16 @@ was useful because it turns out we hadn't written the code to
properly see a change in priority. We fixed this bug before the properly see a change in priority. We fixed this bug before the
code made it into production. code made it into production.
Also notice that some tests include `.IfHasCapability()`. This Line 4: In this example, the next zone adds a second MX record.
limits the test to providers with certain capabilities. You'll To get to this configuration, the provider will have add an
want to use this feature so that the tests only run on providers additional MX record to the same label. New tests don't need to do
that support your new record type. this kind of test because we're pretty sure that part of the diffing
engine work fine. It is here as an example.
Also notice that some tests include `requires()`, `not()` and `only()`
statements. This is how we restrict tests to certain providers.
These options must be listed first in a `testgroup`. More details are
in the source code.
To run the integration test with the BIND provider: To run the integration test with the BIND provider:
@ -173,14 +191,16 @@ To run the integration test with the BIND provider:
go test -v -verbose -provider BIND go test -v -verbose -provider BIND
Once the code works for BIND, consider submitting a PR at this point. Once the code works for BIND, consider submitting a PR at this point.
(The earlier you submit a PR, the earlier we can provide feedback.)
As you debug, if there are places that haven't been marked If you find places that haven't been marked
`#rtype_variations` that should be, add such a comment. `#rtype_variations` but should be, please add that comment.
If you fail to do this, God kills a cute little kitten. Every time you fail to do this, God kills a cute little kitten.
Please do it for the kittens.
## Step 7: Support more providers ## Step 7: Support more providers
Now add support other providers. Add the `providers.CanUse...` Now add support in other providers. Add the `providers.CanUse...`
flag to the provider and re-run the integration tests: flag to the provider and re-run the integration tests:
For example, this will run the tests on Amazon AWS Route53: For example, this will run the tests on Amazon AWS Route53:
@ -196,6 +216,7 @@ code and running the tests. When the tests all work, you are done.
know that everything is working.) know that everything is working.)
If you find bugs that aren't covered by the tests, please please If you find bugs that aren't covered by the tests, please please
please add a test that demonstrates the bug THEN fix the bug. This please add a test that demonstrates the bug (then fix the bug, of
course). This
will help all future contributors. If you need help with adding will help all future contributors. If you need help with adding
tests, please ask! tests, please ask!

View file

@ -224,7 +224,25 @@ FYI: If a provider's capabilities changes, run `go generate` to update
the documentation. the documentation.
## Step 11: Vendor Dependencies ## Step 11: Clean up
Run "go vet" and "golint" and clean up any errors found.
```
go vet
golint
```
Please use `go vet` from the [newest releaes of Go](https://golang.org/doc/devel/release.html#policy).
If [golint](https://github.com/golang/lint) isn't installed on your machine:
```
go get -u golang.org/x/lint/golint
```
## Step 12: Vendor Dependencies
The build process for DNSControl uses the default Go Modules system, The build process for DNSControl uses the default Go Modules system,
which ignores the `vendor` directory. However we store a backup copy which ignores the `vendor` directory. However we store a backup copy
@ -255,12 +273,11 @@ for tips about managing modules and checking for outdated
dependencies. dependencies.
## Step 12: Check your work. ## Step 13: Check your work.
People submitting PRs often forget these steps, so I'll repeat them Here are some last-minute things to check before you submit your PR.
just in case:
1. Run the integration test again just in case. (See Step 7) 1. Run "go generate" to make sure all generated files are fresh.
2. Make sure the documentation is accurate. Verify you didn't miss 2. Make sure all appropriate documentation is current. (See Step 8)
any items in Step 8. 3. Check that dependencies are vendored (See Step 12)
3. Check that dependencies are vendored (See Step 11) 4. Re-run the integration test one last time (See Step 7)

View file

@ -572,9 +572,11 @@ func makeTests(t *testing.T) []*TestGroup {
// Basic functionality (add/rename/change/delete). // Basic functionality (add/rename/change/delete).
// //
testgroup("A", testgroup("GeneralACD",
// These tests aren't specific to "A" records. We're testing // Test general ability to add/change/delete records of one
// general ability to add/rename/change/delete any record. // type. These tests aren't specific to "A" records, but we
// don't do tests specific to A records because this exercises
// them very well.
tc("Create an A record", a("@", "1.1.1.1")), tc("Create an A record", a("@", "1.1.1.1")),
tc("Change it", a("@", "1.2.3.4")), tc("Change it", a("@", "1.2.3.4")),
tc("Add another", a("@", "1.2.3.4"), a("www", "1.2.3.4")), tc("Add another", a("@", "1.2.3.4"), a("www", "1.2.3.4")),
@ -589,11 +591,13 @@ func makeTests(t *testing.T) []*TestGroup {
tc("Delete wildcard", a("www", "1.1.1.1")), tc("Delete wildcard", a("www", "1.1.1.1")),
), ),
//
// Test the basic rtypes.
//
testgroup("CNAME", testgroup("CNAME",
tc("Create a CNAME", cname("foo", "google.com.")), tc("Create a CNAME", cname("foo", "google.com.")),
tc("Change CNAME target", cname("foo", "google2.com.")), tc("Change CNAME target", cname("foo", "google2.com.")),
tc("Change to A record", a("foo", "1.2.3.4")),
tc("Change back to CNAME", cname("foo", "google.com.")),
clear(), clear(),
tc("Record pointing to @", cname("foo", "**current-domain**")), tc("Record pointing to @", cname("foo", "**current-domain**")),
), ),
@ -662,9 +666,17 @@ func makeTests(t *testing.T) []*TestGroup {
), ),
// //
// Tests that exercise the API protocol and/or code // Tests that exercise the API protocol and/or code.
// //
testgroup("TypeChange",
// Test whether the provider properly handles a label changing
// from one rtype to another.
tc("Create a CNAME", cname("foo", "google.com.")),
tc("Change to A record", a("foo", "1.2.3.4")),
tc("Change back to CNAME", cname("foo", "google2.com.")),
),
testgroup("Case Sensitivity", testgroup("Case Sensitivity",
// The decoys are required so that there is at least one actual change in each tc. // The decoys are required so that there is at least one actual change in each tc.
tc("Create CAPS", mx("BAR", 5, "BAR.com.")), tc("Create CAPS", mx("BAR", 5, "BAR.com.")),