-
Notifications
You must be signed in to change notification settings - Fork 119
🐛 Fix nil pointer issue while checking bootstrapReadyCondition #2957
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
We should check if bootstrapReadyCondition is nil or not before checking bootstrapReadyCondition.Status Signed-off-by: Kashif Khan <[email protected]>
|
/test metal3-ubuntu-e2e-integration-test-main metal3-centos-e2e-integration-test-main |
|
@adilGhaffarDev @fmuyassarov @lentzi90 I think this fixes the nil pointer issue as I dont see the error in the controller logs anymore. However, PTAL also yourself and also the worth checking, is it now suppressing any log info. |
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 fixes nil pointer dereference issues when checking bootstrap and infrastructure ready conditions by adding nil checks before accessing condition properties.
- Added nil checks before accessing
Statusfield on conditions retrieved viaGet()method - Introduced test coverage for the scenario where ConfigRef is defined but the condition is not set
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| controllers/metal3machine_controller.go | Added nil check for infrastructureReadyCondition before accessing its Status field |
| baremetal/metal3machine_manager.go | Added nil check for bootstrapReadyCondition before accessing its Status field |
| baremetal/metal3machine_manager_test.go | Added test case to verify behavior when ConfigRef is defined but condition is not set |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lentzi90
left a comment
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 good to me. We will at some point need to add v1beta2 conditions, and then change this so we also check them, but for now I think it is correct.
/approve
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lentzi90 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 |
tuminoid
left a comment
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
Backports?
|
Sorry, it got merged before I had a chance to review it. We need to use sigs.k8s.io/cluster-api/util/deprecated/v1beta1/conditions instead of sigs.k8s.io/cluster-api/util/conditions/deprecated/v1beta1. That should fix the issue. The difference is:
Both packages expose v1beta1 conditions, but one sources them from the v1beta1 API and the other from the v1beta2 API. It’s a bit confusing, but this is how CAPI ensures that v1beta1 behavior continues to work during the transition. |
We should check if bootstrapReadyCondition is nil or not before checking bootstrapReadyCondition.Status
What this PR does / why we need it:
Fixes #2956
Checklist: