Fix panic in api-key delete#3339
Conversation
|
🎉 All Contributor License Agreements have been signed. Ready to merge. |
There was a problem hiding this comment.
Pull request overview
Fixes a panic in the confluent api-key delete command by ensuring the returned error message is derived from the aggregated multierror rather than a potentially-nil error value, particularly when local state (config/keystore) updates fail.
Changes:
- Use
errs.Error()(the aggregated multierror) instead oferr.Error()when constructing the final user-facing error forapi-key delete.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| errs := multierror.Append(err, c.deleteKeysFromKeyStore(deletedIds)) | ||
| if errs.ErrorOrNil() != nil { | ||
| return errors.NewErrorWithSuggestions(err.Error(), errors.ApiKeyNotFoundSuggestions) | ||
| return errors.NewErrorWithSuggestions(errs.Error(), errors.ApiKeyNotFoundSuggestions) |
There was a problem hiding this comment.
errs can include failures from deleteKeysFromKeyStore (which ultimately calls Config.Save()), not just API-key-not-found/forbidden errors. Wrapping all aggregated errors with errors.ApiKeyNotFoundSuggestions will produce misleading guidance when the underlying failure is a local config/keystore write error. Consider branching on the error source (e.g., only attach ApiKeyNotFoundSuggestions when the delete API call failed; otherwise return/wrap the config-save error with a more relevant message/suggestions).
There was a problem hiding this comment.
This is a valid point, but the purpose of the PR is just to triage the panic.
| errs := multierror.Append(err, c.deleteKeysFromKeyStore(deletedIds)) | ||
| if errs.ErrorOrNil() != nil { | ||
| return errors.NewErrorWithSuggestions(err.Error(), errors.ApiKeyNotFoundSuggestions) | ||
| return errors.NewErrorWithSuggestions(errs.Error(), errors.ApiKeyNotFoundSuggestions) |
There was a problem hiding this comment.
There’s no automated regression test covering the scenario described in the PR (API delete succeeds but updating the local config/keystore fails), which previously caused a panic. Please add a unit/integration test that forces Config.Save()/keystore.DeleteAPIKey to error and asserts api-key delete returns a non-nil error (and does not crash).
|
Release Notes
Breaking Changes
New Features
Bug Fixes
confluent api-key deleteto crash if it encounters an error saving while updating the config fileChecklist
Whatsection below whether this PR applies to Confluent Cloud, Confluent Platform, or both.Test & Reviewsection below.Blast Radiussection below.What
The delete command displays a multierror. But there are multiple error variables, and the code was incorrectly calling the
.Error()method on a nil error instead of the multierror, causing it to panic.Blast Radius
Zero. The only change is in the output logic for an error.
References
Test & Review
https://docs.google.com/document/d/1RN9nCxfpDSCTSZILBIn4pDOUS-GyEJ1gjt5_waxi1Q4/edit?usp=sharing