Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Add support for DNS record comments and tags #1158

Conversation

janik-cloudflare
Copy link
Member

Description

This adds support for DNS record comment and tags.

Blog post: https://blog.cloudflare.com/dns-record-comments/
Documentation: https://developers.cloudflare.com/dns/manage-dns-records/reference/record-attributes/

Has your change been tested?

Updated unit tests.

Types of changes

What sort of change does your code introduce/modify?

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • This change is using publicly documented (api.cloudflare.com or developers.cloudflare.com) and stable APIs.

@github-actions
Copy link
Contributor

Oops! It looks like no changelog entry is attached to this PR. Please include a release note as described in https://github.com/cloudflare/cloudflare-go/blob/master/docs/changelog-process.md.

Example:

```release-note:TYPE
Release note
```

If you do not require a release note to be included, please add the workflow/skip-changelog-entry label.

@janik-cloudflare
Copy link
Member Author

Closing in favor of #1151 which looks good and has already been merged!

@favonia
Copy link
Contributor

favonia commented Jan 4, 2023

@janik-cloudflare I have a question about the new DNS record comment API: How should I clear a comment or set the comment to empty without recreating the DNS record?

@janik-cloudflare
Copy link
Member Author

Hi @favonia! Comments can be removed by setting them to null in a PUT or PATCH request.

@favonia
Copy link
Contributor

favonia commented Jan 4, 2023

@janik-cloudflare Thanks. I wonder if it's still possible the change the design to treat the empty string as clearing comment for the following reasons:

  1. The Go interface needs to distinguish the following three cases. Treating "" as clearing has the benefit of reusing Go's built-in JSON encoder for the type *string (with omitempty).
    a. Keep the current comment: nil (which would then be omitted)
    b. Clear the current comment: &""
    c. Set a new comment: &"New comment"
  2. The behavior does not seem to be documented on the API website yet and the API is relatively new. Few, if any, applications depend on this behavior.
  3. If strict backward compatibility is needed, we can treat both "" and null as clearing the comment.

PS: I know &"New comment" is not really a valid Go expression because "New comment" is a constant.

@janik-cloudflare
Copy link
Member Author

@favonia I've discussed this with the team. We agree that the current solution isn't ideal (e.g., Golang makes it too difficult to distinguish between null and an absent key) but we'd prefer not to change the API design to one we find less intuitive in order to work around language limitations.

If a PATCH request with {"comment": ""} removes the comment, I would expect missing comments to always be represented as "" instead of null (in other words, I would be surprised to see {"comment": null} in an API response after sending {"comment": ""}). However, when creating a new record without including the comment field at all, that would require us to send {"comment": ""} in API responses. Although Go treats "" as a default value for strings, most languages either use null or don't have an implicit default at all, so seeing an empty string without ever having set the field would feel unintuitive to me.

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