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

Change UpdateDNSRecord to also return the DNSRecordResponse #1239

Closed
joaoafonsopereira opened this issue Mar 21, 2023 · 4 comments · Fixed by #1243
Closed

Change UpdateDNSRecord to also return the DNSRecordResponse #1239

joaoafonsopereira opened this issue Mar 21, 2023 · 4 comments · Fixed by #1243
Milestone

Comments

@joaoafonsopereira
Copy link
Contributor

Current cloudflare-go version

v0.63.0

Description

Change UpdateDNSRecord return type from just error to (*DNSRecordResponse, error) (and consequently returning the DNSRecordResponse that is already being received)

The proposed change would make this method consistent with:

Use cases

Handling CreateDNSRecord and UpdateDNSRecord's responses in a uniform way.

In my particular case, I need to create/update a DNS record whenever I create/update a worker route (in order to automatically generate a new certificate, as I'm using Total TLS).
Since these pairs of methods (Create/UpdateDNSRecord and Create/UpdateWorkerRoute) are somewhat similar, it would be nice for their signatures/return types to be consistent with each other, enabling one to write code that mirrors those similarities.

Potential cloudflare-go usage

// the main goal is that this function returns a cloudflare.DNSRecord, regardless of whether
// it was created or updated.
func createOrUpdateDNSRecord(ctx context.Context, someParam SomeParam) (cloudflare.DNSRecord, error) {
        // look up dns record somewhere, based on some parameter that doesn't matter
        dnsRecord := findDnsRecord(someParam) 

	proxied := true

	var resp *cloudflare.DNSRecordResponse
	var err error
	if dnsRecord == nil {
		resp, err = cfApi.CreateDNSRecord(ctx, cloudflare.ZoneIdentifier(ZONE_ID), cloudflare.CreateDNSRecordParams{...})
	} else {
		resp, err = cfApi.UpdateDNSRecord(ctx, cloudflare.ZoneIdentifier(ZONE_ID), cloudflare.UpdateDNSRecordParams{...})
	}
        return resp.Result, err
}

References

No response

@jacobbednarz
Copy link
Member

jacobbednarz commented Mar 22, 2023

👋 we're in the process of making the Go library parameters and return types more consistent with our end goal outlined at https://github.com/cloudflare/cloudflare-go/blob/master/docs/experimental.md (recommend taking a look to get the full picture).

i'm open to updating the method signature for Update however, the return type will need to be DNSRecord, not DNSRecordResponse as we are doing for the Create operation.

@joaoafonsopereira
Copy link
Contributor Author

👋
Sorry, missed that document!

No worries there, the essence of this proposal is to make Update and Create's signatures consistent.
Thank you for the quick response, and let me know if you want me to submit a PR implementing those changes.

@jacobbednarz
Copy link
Member

This isn't high on my priority list so if it is something you want, you're welcome to submit a PR.

@github-actions
Copy link
Contributor

This functionality has been released in v0.64.0.

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

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants