-
Notifications
You must be signed in to change notification settings - Fork 593
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: UpdateDNSRecord: clean up params and drop record lookup #1170
dns: UpdateDNSRecord: clean up params and drop record lookup #1170
Conversation
changelog detected ✅ |
92dfa18
to
f1ccdbe
Compare
Codecov Report
@@ Coverage Diff @@
## master #1170 +/- ##
==========================================
- Coverage 49.40% 49.31% -0.10%
==========================================
Files 127 128 +1
Lines 12290 12409 +119
==========================================
+ Hits 6072 6119 +47
- Misses 4840 4886 +46
- Partials 1378 1404 +26
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
f1ccdbe
to
67715bb
Compare
@jacobbednarz the coverage comparison unfortunately is not very helpful because the data for the master branch have not been consistently uploaded to Codecov. |
data is what is used for complex record types (CAA, SRV, etc) instead of
|
This commit does not mean all the remaining fields should be there; I did not touch the ones I do not understand.
833fc1d
to
095641f
Compare
thanks as always, @favonia |
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! |
Description
This PR avoids the extra DNS record lookup for
name
andtype
; these two fields are not needed for the PATCH method and thus the lookup is useless. It also removes many parameters that are probably read-only.PS: This PR does not imply the
data
field should be there---I did not touch it because I do not understand it. Also, I assumemeta
is read-only, which could be wrong. @jacobbednarz Shoulddata
be there? How aboutmeta
?Has your change been tested?
Yes. I can confirm the real Cloudflare API does not need
name
ortype
to update a DNS record.Types of changes
What sort of change does your code introduce/modify?
Even though it changes
UpdateDNSRecordParams
, any existing application which puts non-zero values in those deleted fields should have failed to update the DNS record.Checklist: