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

dns: clean up ListDNSRecordsParams and actually support tags for searching #1173

Merged
merged 3 commits into from
Jan 11, 2023

Conversation

favonia
Copy link
Contributor

@favonia favonia commented Jan 10, 2023

Description

This is the sequel to #1170 to clean up ListDNSRecordsParams.

Has your change been tested?

My (free) Cloudflare plan cannot test the new features tag-match and tag enabled by this PR. Otherwise, this PR passes the minimum test case, which barely tests the tags beyond mocking.

@jacobbednarz @janik-cloudflare Help is needed to confirm the "real" behavior of tags.

Types of changes

What sort of change does your code introduce/modify?

  • Bug fix (non-breaking change which fixes an issue)
    Previously, tags was listed as a parameter but the URL parameter name was tags, which should have been tag.
  • New feature (non-breaking change which adds functionality)
    The parameter tag-match is added.
  • Breaking change (fix or feature that would cause existing functionality to change)
    All the fields not documented in the public API are removed except Priority.

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

github-actions bot commented Jan 10, 2023

changelog detected ✅

@codecov-commenter
Copy link

codecov-commenter commented Jan 10, 2023

Codecov Report

Merging #1173 (5dfa991) into master (6153c1e) will decrease coverage by 0.09%.
The diff coverage is 45.07%.

@@            Coverage Diff             @@
##           master    #1173      +/-   ##
==========================================
- Coverage   49.40%   49.30%   -0.10%     
==========================================
  Files         127      128       +1     
  Lines       12290    12408     +118     
==========================================
+ Hits         6072     6118      +46     
- Misses       4840     4886      +46     
- Partials     1378     1404      +26     
Impacted Files Coverage Δ
access_organization.go 64.70% <ø> (ø)
cloudflare_experimental.go 0.00% <0.00%> (ø)
utils.go 72.72% <ø> (ø)
cloudflare.go 68.37% <14.28%> (-0.34%) ⬇️
mtls_certificates.go 26.59% <26.59%> (ø)
dns.go 66.35% <84.61%> (+2.62%) ⬆️
origin_ca.go 57.26% <90.90%> (-2.09%) ⬇️
devices_managed_networks.go 41.55% <100.00%> (+9.09%) ⬆️
email_routing_destination.go 66.66% <100.00%> (+0.41%) ⬆️
email_routing_rules.go 65.64% <100.00%> (+0.26%) ⬆️
... and 9 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@favonia favonia force-pushed the clean-up-listparams branch 2 times, most recently from 1325f89 to c4598c1 Compare January 10, 2023 13:51
@favonia favonia marked this pull request as ready for review January 10, 2023 14:12
@favonia favonia changed the title dns: clean up ListDNSRecordsParams dns: clean up ListDNSRecordsParams and actually support tags for listing Jan 10, 2023
@favonia favonia changed the title dns: clean up ListDNSRecordsParams and actually support tags for listing dns: clean up ListDNSRecordsParams and actually support tags for searching Jan 10, 2023
@jacobbednarz
Copy link
Member

there are two different uses for tags; with and without predicates.

the first, without, is the simple one where we can do things like /dns_records?tag=foo:bar&tag=team:DNS which searches for an exact match or /dns_records?tag=foo&tag=dns which only searches the presence of those keys without considering the value. this should work as today, example: https://go.dev/play/p/uLmPhZ3SWoA

the second and more difficult one is for predicates (like /dns_records?tag.present=important&tag.exact=team:DNS). i can't see a sensible way of achieving this with go-querystring since map[string]string ends up pretty muddled when it is encoded.

@favonia
Copy link
Contributor Author

favonia commented Jan 11, 2023

@jacobbednarz Got it. Thanks. Then I think this PR correctly implements the basic mode (without predicates).

@jacobbednarz
Copy link
Member

👍 that's totally fine. the non-predicates are shorthand so we can extend the more advance operations later in if needed.

@jacobbednarz jacobbednarz merged commit c136bc1 into cloudflare:master Jan 11, 2023
@github-actions github-actions bot added this to the v0.59.0 milestone Jan 11, 2023
github-actions bot pushed a commit that referenced this pull request Jan 11, 2023
@favonia favonia deleted the clean-up-listparams branch January 11, 2023 22:19
ivan-section-io pushed a commit to section/cloudflare-go that referenced this pull request Jan 12, 2023
@github-actions
Copy link
Contributor

This functionality has been released in v0.59.0.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

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.

3 participants