Summary
Follow-up items from the code review of #213 that were not addressed in #219. These are UX enhancements, refactoring, and test coverage improvements — no bugs.
1. Delete/enable/disable commands only accept IDs, not names
Priority: Low
Files: source_delete.go, source_enable.go, destination_delete.go, destination_enable.go, etc.
get commands support <id-or-name> via the resolve*ID functions, but delete, enable, and disable only accept bare IDs. Users can get my-source but must delete src_abc123.
2. Significant boilerplate across resource types
Priority: Medium
Files: source_list.go, destination_list.go, event_list.go, enable/disable commands, etc.
List commands follow an identical pattern: build params → call client → branch on --output json → format text. Enable/disable commands are nearly identical ~45-line files with only the resource type swapped.
A factory function like makeEnableCmd(resource, clientFn) would eliminate ~200 lines of duplication.
3. Hardcoded magic strings in source_common.go
Priority: Medium
Files: source_common.go:62-64,76,81
Default auth config values like "sha256", "hex", "x-webhook-signature", "x-api-key" are duplicated across buildSourceConfigFromIndividualFlags and normalizeSourceConfigAuth.
Extract to named constants.
4. event_list has 25+ filter flags mapped manually
Priority: Low
Files: event_list.go:100-172
~70 lines of if field != "" { params[key] = field }. A helper like addNonEmpty(params, "key", value) or struct tags would be cleaner and less error-prone.
5. --connection-id maps to API param webhook_id
Priority: Low
Files: event_list.go:104
params["webhook_id"] = ec.connectionID
The mismatch between the user-facing flag name and the API parameter name is confusing. At minimum add a comment; ideally define a constant APIParamConnectionID = "webhook_id".
6. No tests for new resource commands
Priority: Medium
The PR added ~50 new command files but only 2 new test files. Destination, transformation, event, request, and attempt commands have no unit tests.
Remaining items from the code review of #213. See #217 for the original issue; bugs were fixed in #219.
Summary
Follow-up items from the code review of #213 that were not addressed in #219. These are UX enhancements, refactoring, and test coverage improvements — no bugs.
1. Delete/enable/disable commands only accept IDs, not names
Priority: Low
Files:
source_delete.go,source_enable.go,destination_delete.go,destination_enable.go, etc.getcommands support<id-or-name>via theresolve*IDfunctions, butdelete,enable, anddisableonly accept bare IDs. Users canget my-sourcebut mustdelete src_abc123.2. Significant boilerplate across resource types
Priority: Medium
Files:
source_list.go,destination_list.go,event_list.go, enable/disable commands, etc.List commands follow an identical pattern: build params → call client → branch on
--output json→ format text. Enable/disable commands are nearly identical ~45-line files with only the resource type swapped.A factory function like
makeEnableCmd(resource, clientFn)would eliminate ~200 lines of duplication.3. Hardcoded magic strings in
source_common.goPriority: Medium
Files:
source_common.go:62-64,76,81Default auth config values like
"sha256","hex","x-webhook-signature","x-api-key"are duplicated acrossbuildSourceConfigFromIndividualFlagsandnormalizeSourceConfigAuth.Extract to named constants.
4.
event_listhas 25+ filter flags mapped manuallyPriority: Low
Files:
event_list.go:100-172~70 lines of
if field != "" { params[key] = field }. A helper likeaddNonEmpty(params, "key", value)or struct tags would be cleaner and less error-prone.5.
--connection-idmaps to API paramwebhook_idPriority: Low
Files:
event_list.go:104The mismatch between the user-facing flag name and the API parameter name is confusing. At minimum add a comment; ideally define a constant
APIParamConnectionID = "webhook_id".6. No tests for new resource commands
Priority: Medium
The PR added ~50 new command files but only 2 new test files. Destination, transformation, event, request, and attempt commands have no unit tests.
Remaining items from the code review of #213. See #217 for the original issue; bugs were fixed in #219.