Skip to content

dns: swap Priority to *uint16#588

Merged
jacobbednarz merged 1 commit into0.14from
dns-priority-to-uint16-ptr
Feb 22, 2021
Merged

dns: swap Priority to *uint16#588
jacobbednarz merged 1 commit into0.14from
dns-priority-to-uint16-ptr

Conversation

@jacobbednarz
Copy link
Copy Markdown
Contributor

@jacobbednarz jacobbednarz commented Feb 9, 2021

DNSRecord priority was initially an int. This worked most of the time
however in some cases, 0 was intended to be used as a zero value and in
others, a valid value. This causes confusion and in combination with
omitempty on the struct, makes it impossible to send 0 as a value.

type DNSRecord struct {
	Type     string  `json:"type,omitempty"`
	Priority int `json:"priority,omitempty"`
}

func main() {
	// using 0 as a value
	var p = int(0)
	d := DNSRecord{
		Type:     "MX",
		Priority: p,
	}
	b, _ := json.Marshal(d)
	fmt.Println(string(b))

	// => {"type":"MX"}
}

To rectify, I've swapped the struct to *uint16 which accomplishes two
things. The first, aligns with our internal representation of the data
struct. The second, allows us to use 0 in both contexts as a zero value
but also as a value reference where it is needed.

type DNSRecord struct {
	Type     string  `json:"type,omitempty"`
	Priority *uint16 `json:"priority,omitempty"`
}

func main() {
	var p1 = uint16(0)
	d1 := DNSRecord{
		Type:     "MX",
		Priority: &p1,
	}
	b1, _ := json.Marshal(d1)
	fmt.Println(string(b1))
	// => {"type":"MX","priority":0}

	var p2 = uint16(10)
	d2 := DNSRecord{
		Type:     "MX",
		Priority: &p2,
	}
	b2, _ := json.Marshal(d2)
	fmt.Println(string(b2))
	// => {"type":"MX","priority":10}

	d3 := DNSRecord{
		Type:       "MX",
		Priority: nil,
	}
	b3, _ := json.Marshal(d3)
	fmt.Println(string(b3))
	// => {"type":"MX"}
}

Fixes SDK-114 and lays the ground work for the Terraform provider fix.

`DNSRecord` priority was initially an int. This worked most of the time
however in some cases, 0 was intended to be used as a zero value and in
others, a valid value. This causes confusion and in combination with
`omitempty` on the struct, makes it impossible to send 0 as a value.

```go
type DNSRecord struct {
	Type     string  `json:"type,omitempty"`
	Priority int `json:"priority,omitempty"`
}

func main() {
	// using 0 as a value
	var p = int(0)
	d := DNSRecord{
		Type:     "MX",
		Priority: p,
	}
	b, _ := json.Marshal(d)
	fmt.Println(string(b))

	// => {"type":"MX"}
}
```

To rectify, I've swapped the struct to `*uint16` which accomplishes two
things. The first, aligns with our internal representation of the data
struct. The second, allows us to use 0 in both contexts as a zero value
but also as a value reference where it is needed.

```go
type DNSRecord struct {
	Type     string  `json:"type,omitempty"`
	Priority *uint16 `json:"priority,omitempty"`
}

func main() {
	var p1 = uint16(0)
	d1 := DNSRecord{
		Type:     "MX",
		Priority: &p1,
	}
	b1, _ := json.Marshal(d1)
	fmt.Println(string(b1))
	// => {"type":"MX","priority":0}

	var p2 = uint16(10)
	d2 := DNSRecord{
		Type:     "MX",
		Priority: &p2,
	}
	b2, _ := json.Marshal(d2)
	fmt.Println(string(b2))
	// => {"type":"MX","priority":10}

	d3 := DNSRecord{
		Type:       "MX",
		Priority: nil,
	}
	b3, _ := json.Marshal(d3)
	fmt.Println(string(b3))
	// => {"type":"MX"}
}
```

Fixes SDK-114 and lays the ground work for the Terraform provider fix.
@jacobbednarz
Copy link
Copy Markdown
Contributor Author

To add some more context, upstream in Terraform, I'm thinking of the following to allow people to set the priority = 0 in their configuration.

@@ -291,9 +291,9 @@ func resourceCloudflareRecord() *schema.Resource {

 func resourceCloudflareRecordCreate(d *schema.ResourceData, meta interface{}) error {
        client := meta.(*cloudflare.API)
-
+       rrType := d.Get("type").(string)
        newRecord := cloudflare.DNSRecord{
-               Type:    d.Get("type").(string),
+               Type:    rrType,
                Name:    d.Get("name").(string),
                Proxied: d.Get("proxied").(bool),
                ZoneID:  d.Get("zone_id").(string),
@@ -329,8 +329,9 @@ func resourceCloudflareRecordCreate(d *schema.ResourceData, meta interface{}) er
                        valueOk, dataOk)
        }

-       if priority, ok := d.GetOk("priority"); ok {
-               newRecord.Priority = priority.(int)
+       if rrType == "MX" {
+               priority := d.Get("priority").(uint16)
+               newRecord.Priority = &priority
        }

Where rrType is any record type where we require priority to be send to the API regardless of the value.

Copy link
Copy Markdown

@patryk patryk left a comment

Choose a reason for hiding this comment

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

Yeah, I agree with your reasoning. But we would need to release a breaking change release hre to indicate a backwards-incompatible change.

@patryk patryk added this to the v0.14 milestone Feb 9, 2021
@jacobbednarz jacobbednarz changed the base branch from master to 0.14 February 22, 2021 02:20
@jacobbednarz
Copy link
Copy Markdown
Contributor Author

i've pushed a 0.14 branch up and this is now going to merge into that for testing instead of master.

@jacobbednarz jacobbednarz merged commit 2d3f6cd into 0.14 Feb 22, 2021
@jacobbednarz jacobbednarz deleted the dns-priority-to-uint16-ptr branch February 22, 2021 03:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants