Skip to content

fix: remove duplicate keyID on failure and add details when failing to rotate API keys#956

Merged
mbevc1 merged 1 commit into
mainfrom
20260611_fix_key_removal
Jun 11, 2026
Merged

fix: remove duplicate keyID on failure and add details when failing to rotate API keys#956
mbevc1 merged 1 commit into
mainfrom
20260611_fix_key_removal

Conversation

@mbevc1

@mbevc1 mbevc1 commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Couple of small cosmetic fixes, following server fix to properly report API errors now.

@mbevc1 mbevc1 added tech-investment go Pull requests that update go code labels Jun 11, 2026
@claude

claude Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Claude finished @mbevc1's task in 2m 3s —— View job


Review: PR #956 — remove duplicate keyID & add details when failing to rotate

  • Static analysis (build, vet)
  • Review code changes
  • Post review feedback

Static analysis

Check Result
go build ./... ✅ pass
go vet ./cmd/kosli/ ✅ pass

Summary

This is a clean, small fix and the test changes from the latest push resolve the test-breakage I flagged in the previous run — all four golden regexes now match the new error strings:

  • rotate 404 → Error: failed to rotate API key: API key not found
  • delete 404 → failed to delete API key: API key not found
  • both partial-failure suites updated accordingly ✅

I couldn't execute the suite here (the api-key tests run under TestApiKeyCommandTestSuite, whose SetupTest needs the local Kosli server + KOSLI_API_TOKEN_PROD per CLAUDE.md), but I verified statically that each regex matches the new output produced by the changed fmt.Errorf calls.

Notes

  • ✅ Wrapping the rotate error with fmt.Errorf("failed to rotate API key: %w", err) is a genuine improvement — it adds the missing context and makes rotate consistent with delete. The collected-keys-before-failure behaviour is preserved (break then print).
  • 🟡 Minor / non-blocking: removing keyID from the delete message means a multi-key delete no longer names the failing key (reportAlreadyDeleted only lists keys deleted before the failure). The test fixture's server error doesn't carry the ID either. Fine if the real server response now includes the ID — otherwise consider keeping failed to delete API key %s: %w. Left an inline note on deleteApiKey.go:121.

Suggestions

  • Consider adding make lint to your local check before merge — I'm limited to build/vet in this environment.

Overall: 👍 looks good to merge once the multi-key delete identification point above is confirmed as intended.

Comment thread cmd/kosli/deleteApiKey.go
Comment thread cmd/kosli/rotateApiKey.go
@mbevc1 mbevc1 force-pushed the 20260611_fix_key_removal branch 2 times, most recently from 82a4e47 to 5953416 Compare June 11, 2026 12:08
@mbevc1 mbevc1 force-pushed the 20260611_fix_key_removal branch from 5953416 to fcf3ea6 Compare June 11, 2026 12:11
@mbevc1 mbevc1 enabled auto-merge (squash) June 11, 2026 12:12
Comment thread cmd/kosli/deleteApiKey.go
@mbevc1 mbevc1 merged commit 8f399e8 into main Jun 11, 2026
30 of 31 checks passed
@mbevc1 mbevc1 deleted the 20260611_fix_key_removal branch June 11, 2026 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

go Pull requests that update go code tech-investment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants