-
Notifications
You must be signed in to change notification settings - Fork 442
fix AzCluster_default and AzCluster_validation webhooks #5603
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
Conversation
/label tide/merge-method-squash |
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.
Pull Request Overview
This PR primarily addresses code cleanup by removing unused defaulters and updating tests and validation functions. The key changes include:
- Removal of the unused TestControlPlaneOutboundLBTemplateDefaults test case.
- Refactoring test structures and parallel execution in validation tests.
- Correction of typographical and indexing errors in defaulting and validation functions.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
File | Description |
---|---|
api/v1beta1/azureclustertemplate_default_test.go | Removed unused test for control plane outbound LB defaults. |
api/v1beta1/azurecluster_validation_test.go | Updated test struct and looping constructs for clarity. |
api/v1beta1/azurecluster_validation.go | Fixed index usage in security rules validation. |
api/v1beta1/azurecluster_default.go | Corrected function naming typo and constant usage in defaults. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5603 +/- ##
==========================================
- Coverage 53.28% 53.27% -0.02%
==========================================
Files 272 272
Lines 29537 29521 -16
==========================================
- Hits 15739 15727 -12
+ Misses 12983 12979 -4
Partials 815 815 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
/test pull-cluster-api-provider-azure-e2e |
@@ -456,25 +456,6 @@ func (lb *LoadBalancerClassSpec) setOutboundLBDefaults() { | |||
} | |||
} | |||
|
|||
func setControlPlaneOutboundLBDefaults(lb *LoadBalancerClassSpec, apiserverLBType LBType) { |
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.
This function is not used in the webhooks
@@ -1169,93 +1169,6 @@ func TestNodeOutboundLBClassDefaults(t *testing.T) { | |||
} | |||
} | |||
|
|||
func TestControlPlaneOutboundLBTemplateDefaults(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.
This deleted test tested the unused function from above (https://github.com/kubernetes-sigs/cluster-api-provider-azure/pull/5603/files#r2072129130)
/test ls |
@nawazkh: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
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. |
/test pull-cluster-api-provider-azure-e2e-optional |
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
if err := validateSecurityRule( | ||
rule, | ||
fldPath.Index(i).Child("securityGroup").Child("securityRules").Index(i), | ||
fldPath.Index(i).Child("securityGroup").Child("securityRules").Index(j), |
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.
Looks like we had a bug here...
LGTM label has been added. Git tree hash: c355ffc1591f57d9efb6740ee4dd185ce3b3db74
|
Could you |
/retitle fix AzCluster_default and AzCluster_validation webhooks |
/assign @willie-yao |
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
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: willie-yao 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 |
Even though this PR is a cleanup type, one of the webhooks was behaving weirdly(bug), so it qualifies to be cherry-picked.. |
@nawazkh: new pull request created: #5616 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. |
@nawazkh: new pull request created: #5617 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. |
…igs#5603) * remove unused defaulters * fix code * fix tests
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
TODOs:
Release note: