Add bulk key operations: bulk-create, bulk-update, bulk-delete#212
Add bulk key operations: bulk-create, bulk-update, bulk-delete#212lechuckcaptain wants to merge 4 commits intolokalise:mainfrom
Conversation
Expose the SDK's BulkUpdate, BulkDelete, and multi-key Create methods as new CLI subcommands, closing the gap between API and CLI capabilities.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: lokalise/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds three new CLI subcommands under Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmd/key_test.go (1)
51-79: Strengthen update/create tests with outbound payload assertions.Lines 51-79 and 97-141 only verify method/header. Adding
testBody(...)checks (like Line 27 in bulk-delete) would catch regressions in request marshaling for the two new commands.Also applies to: 97-141
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/key_test.go` around lines 51 - 79, The PUT and the corresponding create handler in cmd/key_test.go currently only assert method and header; add testBody assertions (using the existing testBody(t, r, expectedJSON) helper like in the bulk-delete test) to both handlers to validate the outbound request payloads for the update and create commands; locate the mux.HandleFunc blocks that use fmt.Sprintf("/api2/projects/%s/keys", testProjectID) and the second keys handler near the other test and add testBody calls with the expected JSON payload (matching what the CLI should send) immediately after testHeader so the request marshaling is asserted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/key.go`:
- Around line 22-26: The shared package-level boolean useAutomations is being
bound to multiple Cobra commands (keyCreateCmd, keyBulkUpdateCmd,
keyBulkCreateCmd) causing flag state to leak across commands; introduce distinct
backing variables (e.g., useAutomationsCreate, useAutomationsBulkUpdate,
useAutomationsBulkCreate), bind each command's --use-automations flag to its own
variable, and update the places that read the flag (the handlers referencing
useAutomations) to use the corresponding new variable for keyCreateCmd,
keyBulkUpdateCmd, and keyBulkCreateCmd so each command's flag is decoupled.
- Line 24: Change the bulkDeleteKeyIds declaration from []int to []int64 and
remove the manual conversion that builds a []int64 before the API call; update
the CLI flag registration to use Int64SliceVar instead of IntSliceVar so the
flag directly populates bulkDeleteKeyIds. Specifically, modify the variable
named bulkDeleteKeyIds, delete the conversion loop that casts []int to []int64,
ensure places that consume bulkDeleteKeyIds (the bulk delete handler) accept
[]int64, and replace any flag usage of IntSliceVar with Int64SliceVar so
single-key operations (keyId) and bulk operations use int64 end-to-end.
---
Nitpick comments:
In `@cmd/key_test.go`:
- Around line 51-79: The PUT and the corresponding create handler in
cmd/key_test.go currently only assert method and header; add testBody assertions
(using the existing testBody(t, r, expectedJSON) helper like in the bulk-delete
test) to both handlers to validate the outbound request payloads for the update
and create commands; locate the mux.HandleFunc blocks that use
fmt.Sprintf("/api2/projects/%s/keys", testProjectID) and the second keys handler
near the other test and add testBody calls with the expected JSON payload
(matching what the CLI should send) immediately after testHeader so the request
marshaling is asserted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: lokalise/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4659508e-21cc-4378-a17b-89487b073f3e
📒 Files selected for processing (2)
cmd/key.gocmd/key_test.go
…ions - Decouple use-automations flag by introducing per-command backing variables (bulkUpdateUseAutomations, bulkCreateUseAutomations) to prevent flag state leaking across commands - Change bulkDeleteKeyIds from []int to []int64 with Int64SliceVar, removing the manual conversion loop for consistency with single-key operations - Add testBody assertions to TestKeyBulkUpdate and TestKeyBulkCreate to validate outbound request payloads
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cmd/key.go (1)
3-8: Harden--keysvalidation and error messages before API callsBoth bulk handlers currently return raw unmarshal errors and accept
[]silently. Add CLI-side validation so users get actionable errors and empty payloads fail fast.💡 Proposed refactor
import ( "encoding/json" + "fmt" "github.com/lokalise/go-lokalise-api/v4" "github.com/spf13/cobra" ) @@ RunE: func(*cobra.Command, []string) error { var keys []lokalise.BulkUpdateKey if err := json.Unmarshal([]byte(bulkUpdateKeys), &keys); err != nil { - return err + return fmt.Errorf("invalid value for --keys: expected JSON array of bulk update keys: %w", err) + } + if len(keys) == 0 { + return fmt.Errorf("--keys must contain at least one key object") } @@ RunE: func(*cobra.Command, []string) error { var keys []lokalise.NewKey if err := json.Unmarshal([]byte(bulkCreateKeys), &keys); err != nil { - return err + return fmt.Errorf("invalid value for --keys: expected JSON array of new keys: %w", err) + } + if len(keys) == 0 { + return fmt.Errorf("--keys must contain at least one key object") }Also applies to: 160-163, 182-185
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/key.go` around lines 3 - 8, The CLI should validate the --keys flag payload before making API calls: locate the code that reads the --keys flag and calls json.Unmarshal (the bulk handlers that unmarshal into a keys slice) and add pre-checks that the raw string is non-empty and not just "[]" (trim whitespace and reject if empty or equals "[]"); then perform json.Unmarshal but wrap any error with a user-friendly message like "failed to parse --keys JSON: <err>" and after unmarshalling verify the resulting slice length > 0 and return a clear error ("--keys payload must contain at least one key") instead of letting empty slices or raw unmarshal errors propagate to the API. Ensure these checks are applied to both bulk handlers that currently perform json.Unmarshal so the API is never called with an empty payload.cmd/key_test.go (1)
47-96: Add explicit--use-automations=falsecoverageRight now only the default
truepath is asserted. Add one test each forbulk-updateandbulk-createwith--use-automations=false, and assert"use_automations":falsein the request body to lock in the flag behavior.Also applies to: 98-163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/key_test.go` around lines 47 - 96, Add new test cases alongside TestKeyBulkUpdate to cover the explicit false path for both bulk-update and bulk-create: duplicate the existing TestKeyBulkUpdate pattern (and the bulk-create test around lines 98-163) to create TestKeyBulkUpdate_UseAutomationsFalse and TestKeyBulkCreate_UseAutomationsFalse; set args to include "--use-automations=false" (e.g. args := []string{"key","bulk-update","--keys=...","--project-id="+testProjectID,"--use-automations=false"}) and in the mux handler for each test assert the request body contains `"use_automations":false` (use the same json compact + testBody approach used in TestKeyBulkUpdate), and ensure keyBulkUpdateCmd.PreRun / keyBulkCreateCmd.PreRun set Api = client before executing rootCmd.Execute() so the client is used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/key_test.go`:
- Around line 47-96: Add new test cases alongside TestKeyBulkUpdate to cover the
explicit false path for both bulk-update and bulk-create: duplicate the existing
TestKeyBulkUpdate pattern (and the bulk-create test around lines 98-163) to
create TestKeyBulkUpdate_UseAutomationsFalse and
TestKeyBulkCreate_UseAutomationsFalse; set args to include
"--use-automations=false" (e.g. args :=
[]string{"key","bulk-update","--keys=...","--project-id="+testProjectID,"--use-automations=false"})
and in the mux handler for each test assert the request body contains
`"use_automations":false` (use the same json compact + testBody approach used in
TestKeyBulkUpdate), and ensure keyBulkUpdateCmd.PreRun / keyBulkCreateCmd.PreRun
set Api = client before executing rootCmd.Execute() so the client is used.
In `@cmd/key.go`:
- Around line 3-8: The CLI should validate the --keys flag payload before making
API calls: locate the code that reads the --keys flag and calls json.Unmarshal
(the bulk handlers that unmarshal into a keys slice) and add pre-checks that the
raw string is non-empty and not just "[]" (trim whitespace and reject if empty
or equals "[]"); then perform json.Unmarshal but wrap any error with a
user-friendly message like "failed to parse --keys JSON: <err>" and after
unmarshalling verify the resulting slice length > 0 and return a clear error
("--keys payload must contain at least one key") instead of letting empty slices
or raw unmarshal errors propagate to the API. Ensure these checks are applied to
both bulk handlers that currently perform json.Unmarshal so the API is never
called with an empty payload.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: lokalise/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c06b2c44-b96d-4157-a628-ca1b08008d17
📒 Files selected for processing (2)
cmd/key.gocmd/key_test.go
Reject empty JSON arrays for --keys on bulk-update and bulk-create before hitting the API. Add unit tests for --use-automations=false path and empty keys validation.
|
@coderabbitai review 🙏 |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (3)
cmd/key.go (1)
143-154: Consider validating non-empty key IDs for consistency.Unlike
bulk-updateandbulk-createwhich validatelen(keys) == 0,bulk-deletedoesn't check for an emptybulkDeleteKeyIdsslice. While the flag is marked required, passing--key-ids=could result in an empty slice being sent to the API.♻️ Suggested validation for consistency
var keyBulkDeleteCmd = &cobra.Command{ Use: "bulk-delete", Short: "Delete multiple keys", Long: "Deletes multiple keys from the project. Requires Manage keys admin right.", RunE: func(*cobra.Command, []string) error { + if len(bulkDeleteKeyIds) == 0 { + return errors.New("--key-ids must contain at least one key ID") + } resp, err := Api.Keys().BulkDelete(projectId, bulkDeleteKeyIds)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/key.go` around lines 143 - 154, The keyBulkDeleteCmd handler should validate that bulkDeleteKeyIds is non-empty before calling Api.Keys().BulkDelete: add the same check used in bulk-update/bulk-create to return an error when len(bulkDeleteKeyIds) == 0 (or when the parsed flag results in an empty slice) and avoid calling Api.Keys().BulkDelete/printJson with an empty list; reference the keyBulkDeleteCmd, the bulkDeleteKeyIds variable, and the Api.Keys().BulkDelete invocation when making the change.cmd/key_test.go (2)
36-44: Consider resetting command state between tests for better isolation.Tests mutate shared Cobra command state (
rootCmd.SetArgs(),*.PreRun). While each test explicitly sets all required args, flag values from previous tests could persist if a test fails mid-execution. This is a broader pattern in the codebase, not specific to this PR.A common mitigation is to reset flags or use subtests with fresh command instances. However, since this follows existing patterns in the repository, it's acceptable to defer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/key_test.go` around lines 36 - 44, The test mutates shared Cobra state (rootCmd.SetArgs and keyBulkDeleteCmd.PreRun) which can leak between tests; fix by saving and restoring the mutated state around the test: capture the original rootCmd args (call rootCmd.Args or the current args slice) and original keyBulkDeleteCmd.PreRun, then use defer to restore them after the test, or explicitly clear args with rootCmd.SetArgs(nil) and reset keyBulkDeleteCmd.PreRun = nil at the end of the test to ensure isolation for functions rootCmd.SetArgs and keyBulkDeleteCmd.PreRun.
233-295: Consider asserting specific error messages for robustness.The error path tests verify that an error occurs but don't validate the error content. This could mask regressions where the command fails for unexpected reasons.
♻️ Example: validate error message for empty keys test
err := rootCmd.Execute() if err == nil { t.Error("Expected error for empty keys, got nil") } + if err != nil && err.Error() != "--keys must contain at least one key object" { + t.Errorf("Expected empty keys error, got: %v", err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/key_test.go` around lines 233 - 295, The tests TestKeyBulkUpdate_InvalidJSON, TestKeyBulkCreate_InvalidJSON, TestKeyBulkUpdate_EmptyKeys and TestKeyBulkCreate_EmptyKeys only check that an error occurred but not that it is the expected error; update each test that calls rootCmd.Execute (with keyBulkUpdateCmd.PreRun or keyBulkCreateCmd.PreRun) to assert the returned error message or type matches the expected failure (e.g., "invalid JSON" for malformed --keys and "no keys provided" or similar for empty arrays) using t.Fatalf/t.Errorf or your test helper (require/expect) so the tests fail if the command errors for an unexpected reason; reference the specific commands keyBulkUpdateCmd and keyBulkCreateCmd and validate err.Error() (or wrapped error check) against the canonical message/identifier your CLI returns.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/key_test.go`:
- Around line 36-44: The test mutates shared Cobra state (rootCmd.SetArgs and
keyBulkDeleteCmd.PreRun) which can leak between tests; fix by saving and
restoring the mutated state around the test: capture the original rootCmd args
(call rootCmd.Args or the current args slice) and original
keyBulkDeleteCmd.PreRun, then use defer to restore them after the test, or
explicitly clear args with rootCmd.SetArgs(nil) and reset
keyBulkDeleteCmd.PreRun = nil at the end of the test to ensure isolation for
functions rootCmd.SetArgs and keyBulkDeleteCmd.PreRun.
- Around line 233-295: The tests TestKeyBulkUpdate_InvalidJSON,
TestKeyBulkCreate_InvalidJSON, TestKeyBulkUpdate_EmptyKeys and
TestKeyBulkCreate_EmptyKeys only check that an error occurred but not that it is
the expected error; update each test that calls rootCmd.Execute (with
keyBulkUpdateCmd.PreRun or keyBulkCreateCmd.PreRun) to assert the returned error
message or type matches the expected failure (e.g., "invalid JSON" for malformed
--keys and "no keys provided" or similar for empty arrays) using
t.Fatalf/t.Errorf or your test helper (require/expect) so the tests fail if the
command errors for an unexpected reason; reference the specific commands
keyBulkUpdateCmd and keyBulkCreateCmd and validate err.Error() (or wrapped error
check) against the canonical message/identifier your CLI returns.
In `@cmd/key.go`:
- Around line 143-154: The keyBulkDeleteCmd handler should validate that
bulkDeleteKeyIds is non-empty before calling Api.Keys().BulkDelete: add the same
check used in bulk-update/bulk-create to return an error when
len(bulkDeleteKeyIds) == 0 (or when the parsed flag results in an empty slice)
and avoid calling Api.Keys().BulkDelete/printJson with an empty list; reference
the keyBulkDeleteCmd, the bulkDeleteKeyIds variable, and the
Api.Keys().BulkDelete invocation when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: lokalise/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 92c0f63a-ef91-4c4d-b0ef-94f97dc2ed23
📒 Files selected for processing (2)
cmd/key.gocmd/key_test.go
lechuckcaptain
force-pushed
the
feature/bulk-key-operations
branch
from
e66a3cc to
2b9a549
Compare
1 participant
Problem
The Lokalise API supports bulk operations on keys — creating multiple keys in a single request, updating multiple keys at once, and deleting multiple keys by ID. The underlying Go SDK (
go-lokalise-api/v4) already exposes these capabilities:Keys().Create(projectID, []NewKey{...})— accepts a slice of keysKeys().BulkUpdate(projectID, []BulkUpdateKey{...})— dedicated bulk update methodKeys().BulkDelete(projectID, []int64{...})— dedicated bulk delete methodHowever, the CLI only wraps these as single-item operations:
key createwraps oneNewKeyin a slicekey updatecallsKeys().Update()(single key only)key deletecallsKeys().Delete()(single key only)This forces users who need to operate on many keys to loop over individual CLI calls, resulting in
NAPI requests instead of1. For projects with hundreds or thousands of keys, this is a significant usability and performance gap — especially for common workflows like batch tagging, archiving unused keys, or bootstrapping a new project with a predefined key set.Solution
Three new subcommands under
key:key bulk-deleteAccepts a comma-separated list of key IDs and calls
Keys().BulkDelete().key bulk-updateAccepts a JSON array of
BulkUpdateKeyobjects (each containingkey_idand fields to update) and callsKeys().BulkUpdate().key bulk-createAccepts a JSON array of
NewKeyobjects and callsKeys().Create()with the full slice.Both
bulk-updateandbulk-createsupport the existing--use-automationsflag (defaulttrue).Design decisions
bulk-deleteandbulk-updatemap to distinct SDK methods with different signatures. Keeping them separate avoids backward-compatibility concerns and keeps each command focused.--key-idsas IntSlice for bulk-delete — follows the existing pattern fromteam-user-group add-members(IntSliceVar+int-to-int64conversion).--keysas JSON string for bulk-update/create — follows the existing pattern of JSON string flags (--translations,--filenames). For large payloads, users can use--keys "$(cat keys.json)".key create,key update, andkey deleteare not modified. Zero breaking changes.Test plan
cmd/key_test.go) covering all three commands + invalid JSON error handlinggo test ./...passesgo build ./...passesbulk-createcreated 2 keys in one API callbulk-updateupdated descriptions and tags on both keys in one callbulk-deletedeleted both keys in one call (keys_removed: true)