chore(cloudflare): migrate customhostname to v5 #5891
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/XXL
Pull Request Test Coverage Report for Build 18617256505Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Moved all tests from cloudflare_customhostname_test.go into cloudflare_test.go to ensure proper coverage tracking by coveralls. Tests maintain 92.4% coverage.
AndrewCharlesHay
changed the title
Co-authored-by: Michel Loiseleur <[email protected]>
|
@vflaux @mrozentsvayg Any comment on this PR ? Do you think you can review it ? |
|
@vflaux: changing LGTM is restricted to collaborators In response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Co-authored-by: vflaux <[email protected]>
Co-authored-by: vflaux <[email protected]>
…Flare API - Change CustomHostnames method signature to return autoPager[CustomHostnameListResponse] instead of processed []CustomHostname for better API matching and testability - Keep listAllCustomHostnames helper function for callers that need processed results - Update mock implementation to return autoPager with realistic CloudFlare API behavior - All tests continue to pass with improved interface design Addresses PR feedback about method signatures being closer to CloudFlare API for better mocking.
- Handle CloudFlare v5 SDK errors according to official documentation - Use errors.As() to check for cloudflare.Error type - Convert 429 (rate limit) status codes to SoftError for retry logic - Add comprehensive test coverage for v5 error handling - Use wrapper types in tests to avoid CloudFlare SDK internal nil pointer issues Addresses PR comment on line 376 requesting proper v5 error handling.
|
How did broken tests get merged in? |
This should be fixed, see #5896. |
|
/retest |
|
@vflaux everything look good now? |
| if client == nil { | ||
| return nil, fmt.Errorf("failed to initialize cloudflare provider") | ||
| } |
There was a problem hiding this comment.
I's unreachable, right?
| if client == nil { | |
| return nil, fmt.Errorf("failed to initialize cloudflare provider") | |
| } |
| func (z zoneService) CreateCustomHostname(ctx context.Context, zoneID string, ch CustomHostname) error { | ||
| params := custom_hostnames.CustomHostnameNewParams{ | ||
| ZoneID: cloudflare.F(zoneID), | ||
| Hostname: cloudflare.F(ch.Hostname), | ||
| } | ||
|
|
||
| if ch.SSL != nil { | ||
| sslParams := custom_hostnames.CustomHostnameNewParamsSSL{} | ||
| if ch.SSL.Method != "" { | ||
| sslParams.Method = cloudflare.F(custom_hostnames.DCVMethod(ch.SSL.Method)) | ||
| } | ||
| if ch.SSL.Type != "" { | ||
| sslParams.Type = cloudflare.F(custom_hostnames.DomainValidationType(ch.SSL.Type)) | ||
| } | ||
| if ch.SSL.BundleMethod != "" { | ||
| sslParams.BundleMethod = cloudflare.F(custom_hostnames.BundleMethod(ch.SSL.BundleMethod)) | ||
| } | ||
| if ch.SSL.CertificateAuthority != "" && ch.SSL.CertificateAuthority != "none" { | ||
| sslParams.CertificateAuthority = cloudflare.F(cloudflare.CertificateCA(ch.SSL.CertificateAuthority)) | ||
| } | ||
| if ch.SSL.Settings.MinTLSVersion != "" { | ||
| sslParams.Settings = cloudflare.F(custom_hostnames.CustomHostnameNewParamsSSLSettings{ | ||
| MinTLSVersion: cloudflare.F(custom_hostnames.CustomHostnameNewParamsSSLSettingsMinTLSVersion(ch.SSL.Settings.MinTLSVersion)), | ||
| }) | ||
| } | ||
| params.SSL = cloudflare.F(sslParams) | ||
| } | ||
|
|
||
| _, err := z.service.CustomHostnames.New(ctx, params) | ||
| return err |
There was a problem hiding this comment.
Is it possible to do the same you've done with CustomHostnames() ?
Signature close to the cloudflare api & extract the build of CustomHostnameNewParams{} to another func + tests ?
| }) | ||
| } | ||
|
|
||
| func TestBuildCustomHostnameSSLParams(t *testing.T) { |
There was a problem hiding this comment.
Not sure what this is supposed to test 😄.
This doesn't call any function from this package.
| var foundID string | ||
| for _, h := range hostnames { | ||
| if h.Hostname == "test.example.com" { | ||
| foundID = h.ID | ||
| break | ||
| } | ||
| } | ||
| assert.Equal(t, "ch1", foundID) |
There was a problem hiding this comment.
No need to loop over the list as there is only one element.
| var foundID string | |
| for _, h := range hostnames { | |
| if h.Hostname == "test.example.com" { | |
| foundID = h.ID | |
| break | |
| } | |
| } | |
| assert.Equal(t, "ch1", foundID) | |
| assert.Equal(t, "ch1", hostnames[0].ID) |
| } | ||
| }) | ||
|
|
||
| t.Run("IteratorError", func(t *testing.T) { |
There was a problem hiding this comment.
This is case is very similar to the next one (PartialIteratorError). I'm not sure if this one is needed?
| } | ||
|
|
||
| // TestZoneIDByName tests the ZoneIDByName function | ||
| func TestZoneIDByName(t *testing.T) { |
There was a problem hiding this comment.
I think it's unrelated to this PR and add nothing to the current tests.
This test provider.Zones() which is already covered by TestCloudFlareZones*() tests.
There was a problem hiding this comment.
Yeah sorry I couldn't figure out how to rerun the tests and didn't want to do an empty commit. I'll remove it
Co-authored-by: vflaux <[email protected]>
5 participants
What does it do ?
Migrate customhostname to v5
Motivation
Closes #5540
More