-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Bump Cloudflare-Go #2298
Bump Cloudflare-Go #2298
Conversation
TTL: 120, | ||
Content: "1.2.3.4", | ||
Proxied: test.RecordsAreProxied, | ||
t.Run(test.Name, func(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decided to do this as it was hard to figure out what test case was failing.
provider/cloudflare/cloudflare.go
Outdated
// see: https://github.com/cloudflare/cloudflare-go/pull/595 | ||
|
||
// ProxyEnabled is a pointer to a bool true showing the record should be proxied through cloudflare | ||
var ProxyEnabled *bool = &[]bool{true}[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tbh im not really happy about this message thing of creating a slice of bool, setting first element to true, and getting that element.
i wish there was a better way to do this, and am 500% all ears.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👋🏻 WDYT about creating a helper function which return a bool pointer? I think this used in a lot of packages (IIRC aws-sdk-go definitely has this), it's a bit more readable IMHO.
func Bool() *bool {
b := true
return &b
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should be also proxyEnabled
.
Hm, this breaks when doing local testing unfortunately:
will look further |
Looks like they made https://github.com/cloudflare/cloudflare-go/blob/v0.22.0/zone.go#L441-L443 Introduced in cloudflare/cloudflare-go#674 Should be ready for review. |
In order for us to use some new features provided by cloudflare-go for the cloudflare provider (mainly around Cloudflare Access, etc.) we need to bump the version of cloudflare-go used. The only real change that needed to be done was that the `Proxied` field on `cloudflare.DNSRecord` structs changed from `bool` -> `*bool`. Upstream did this because in certain cases, a user could not flip from proxied -> DNS only (the library omitted sending `proxied = false` to the API) Other decent change was that most methods exported by the library now require `context.Context` to be passed in. Was rather trivial to get up. cloudflare/cloudflare-go#595 Signed-off-by: Devon Mizelle <devon.mizelle@onepeloton.com>
a4edf79
to
369376f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nits, but lgtm.
provider/cloudflare/cloudflare.go
Outdated
// see: https://github.com/cloudflare/cloudflare-go/pull/595 | ||
|
||
// ProxyEnabled is a pointer to a bool true showing the record should be proxied through cloudflare | ||
var ProxyEnabled *bool = &[]bool{true}[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👋🏻 WDYT about creating a helper function which return a bool pointer? I think this used in a lot of packages (IIRC aws-sdk-go definitely has this), it's a bit more readable IMHO.
func Bool() *bool {
b := true
return &b
}
provider/cloudflare/cloudflare.go
Outdated
// see: https://github.com/cloudflare/cloudflare-go/pull/595 | ||
|
||
// ProxyEnabled is a pointer to a bool true showing the record should be proxied through cloudflare | ||
var ProxyEnabled *bool = &[]bool{true}[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should be also proxyEnabled
.
provider/cloudflare/cloudflare.go
Outdated
var ProxyEnabled *bool = &[]bool{true}[0] | ||
|
||
// ProxyDisabled is a pointer to a bool false showing the record should not be proxied through cloudflare | ||
var ProxyDisabled *bool = &[]bool{false}[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use !proxyEnabled
instead or do you think it's more readable having proxyDisabled
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think its more readable/explicit this way
@@ -177,24 +186,20 @@ func (p *CloudFlareProvider) Zones(ctx context.Context) ([]cloudflare.Zone, erro | |||
} | |||
|
|||
log.Debugln("no zoneIDFilter configured, looking at all zones") | |||
for { | |||
zonesResponse, err := p.Client.ListZonesContext(ctx, cloudflare.WithPagination(p.PaginationOptions)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there no pagination needed anymore? We had to implement it if you'd have more than x zones in Cloudflare otherwise it wouldn't change all DNS zones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! See #2298 (comment)
Makes it easier for us :)
go.mod
Outdated
@@ -17,7 +17,7 @@ require ( | |||
github.com/aliyun/alibaba-cloud-sdk-go v1.61.357 | |||
github.com/aws/aws-sdk-go v1.40.38 | |||
github.com/bodgit/tsig v0.0.2 | |||
github.com/cloudflare/cloudflare-go v0.13.2 | |||
github.com/cloudflare/cloudflare-go v0.22.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also can we bump this to 0.25.0? I saw dependabot wanted to update it.
* Bump cloudflare-go to 0.25.0 * Use a function for pointer booleans rather than hardcoding it.
hey @njuettner -- thanks for the review. updated with a new commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dmizelle, njuettner The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Description
In order for us to use some new features provided by cloudflare-go for
the cloudflare provider (mainly around Cloudflare Access, etc.) we need
to bump the version of cloudflare-go used.
The only real change that needed to be done was that the
Proxied
fieldon
cloudflare.DNSRecord
structs changed frombool
->*bool
.Upstream did this because in certain cases, a user could not flip from
proxied -> DNS only (the library omitted sending
proxied = false
tothe API)
Other decent change was that most methods exported by the library now
require
context.Context
to be passed in. Was rather trivial to get up.cloudflare/cloudflare-go#595
Signed-off-by: Devon Mizelle devon.mizelle@onepeloton.com
Checklist