cluster-api-provider-aws
cluster-api-provider-aws copied to clipboard
kubernetes-sigs
🐛 fix(taint deletion): allow taint changes and prevent panic by handling nil values
This commit addresses a panic error that occurs when attempting to delete a taint in AWSManagedMachinePool. The issue was caused by a nil pointer dereference when the taint's value was nil. By handling nil values for taint fields, this fix ensures that taints with nil values are correctly processed, preventing runtime errors.
Fixes #5021
What type of PR is this?
/kind bug
What this PR does / why we need it:
This PR resolves a panic error that occurs when attempting to delete a taint in AWSManagedMachinePool. The error was due to a nil pointer dereference when the taint's value was nil. By handling nil values for taint fields, this fix ensures that taints with nil values are correctly processed, preventing runtime errors.
Which issue(s) this PR fixes: Fixes #5021
Special notes for your reviewer:
Checklist:
- [X] squashed commits
- [ ] includes documentation
- [x] includes emojis
- [x] adds unit tests
- [x] adds or updates e2e tests
Release note:
Fixed a panic error during taint deletion in AWSManagedMachinePool by handling nil values for taint fields.
Hi @nueavv. Thanks for your PR.
I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.
Once the patch is verified, the new status will be reflected by the ok-to-test label.
I understand the commands that are listed here.
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.
Thanks for the this @nueavv . When you have time could you respond to @fiunchinho suggestion? Then i think we can look to get this merged.
@nueavv - there's a couple of failures with the checks. If you run the following locally (which is what he checks are running):
make lintmake test
Ping me on slack if i can help with anything.
@richardcase I have fixed the issue. Can you please check if the commit message is appropriate?
@richardcase: The following commands are available to trigger required jobs:
/test pull-cluster-api-provider-aws-build/test pull-cluster-api-provider-aws-build-docker/test pull-cluster-api-provider-aws-test/test pull-cluster-api-provider-aws-verify
The following commands are available to trigger optional jobs:
/test pull-cluster-api-provider-aws-apidiff-main/test pull-cluster-api-provider-aws-e2e/test pull-cluster-api-provider-aws-e2e-blocking/test pull-cluster-api-provider-aws-e2e-clusterclass/test pull-cluster-api-provider-aws-e2e-conformance/test pull-cluster-api-provider-aws-e2e-conformance-with-ci-artifacts/test pull-cluster-api-provider-aws-e2e-eks/test pull-cluster-api-provider-aws-e2e-eks-gc/test pull-cluster-api-provider-aws-e2e-eks-testing
Use /test all to run the following jobs that were automatically triggered:
pull-cluster-api-provider-aws-apidiff-mainpull-cluster-api-provider-aws-buildpull-cluster-api-provider-aws-build-dockerpull-cluster-api-provider-aws-testpull-cluster-api-provider-aws-verify
In response to this:
/test ?
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.
Hi @AndiDog,
I was trying to delete the taint, but it failed because of the empty string. What made you confused? The issue is about taint deletion.
You can reproduce it by first creating a taint without a value and then trying to delete it.
The change now looks straightforward. Can you please make this a single commit? I'll take a look very soon – it's on my list 😉.
/approve /hold
@fiunchinho @dlipovetsky @richardcase does someone have time to review and LGTM? Please /unhold once all is okay.
Thanks for this. The change looks good.
I noticed this file has no unit tests. In my experience, unit tests are especially helpful for functions that transform data. Would you please consider writing a unit test; it should fail without your fix, and pass with it.
On second thought, this PR has been open for a while, and I don't want to add a new requirement this late. But a unit test would help future maintenance. If it can be done before the /hold is removed, great; otherwise, a follow-up PR would be fine.
/lgtm
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.
This bot triages PRs according to the following rules:
- After 90d of inactivity,
lifecycle/staleis applied - After 30d of inactivity since
lifecycle/stalewas applied,lifecycle/rottenis applied - After 30d of inactivity since
lifecycle/rottenwas applied, the PR is closed
You can:
- Mark this PR as fresh with
/remove-lifecycle stale - Close this PR with
/close - Offer to help out with Issue Triage
Please send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale
This has been open a very long time, sorry @nueavv.
Any chance you could rebase and we can get this in? Also, might be worth making the change @AndiDog mentions.