Skip to content

Migrate webhook commands to SDK#159

Merged
robzolkos merged 4 commits into
masterfrom
migrate-webhook-to-sdk
Apr 30, 2026
Merged

Migrate webhook commands to SDK#159
robzolkos merged 4 commits into
masterfrom
migrate-webhook-to-sdk

Conversation

@robzolkos
Copy link
Copy Markdown
Collaborator

@robzolkos robzolkos commented Apr 29, 2026

Summary

  • Migrates webhook show|create|update|delete|reactivate from the legacy HTTP client (getClient()) to the typed WebhooksService on the SDK account client.
  • webhook list uses the typed service for the no-flag case and falls back to ac.Get / ac.GetAll for --page and --all, since the SDK's Webhooks().List doesn't accept a path. (webhook deliveries was already on the SDK and is untouched.)
  • Behavior, flags, and CLI surface are unchanged — SURFACE.txt doesn't change. The e2e CRUD test in webhook_test.go continues to drive the same UX.

Notes

  • Unit tests switched to the SetTestModeWithSDK httptest-backed pattern.
  • Body assertions updated: SDK posts the typed request directly, so no more {"webhook": {...}} wrapper. URL assertions for get/update/delete drop the .json suffix to match the SDK paths (Rails accepts both forms).
  • Create no longer needs a Location follow-up — WebhooksService.Create returns the full Webhook directly.

Summary by cubic

Migrates webhook CLI commands from the legacy HTTP client to the typed SDK for better type safety with the same CLI behavior. Also fixes list paging when using --all with --page.

  • Refactors

    • Switched to SDK WebhooksService for show, create, update, delete, and reactivate.
    • list: uses Webhooks().List by default; falls back to ac.Get/ac.GetAll for --page/--all.
    • Request/response: removed "webhook" body wrapper; dropped .json paths; Create returns full Webhook (no Location follow-up); removed unused printSuccessWithLocation.
    • Tests now use SetTestModeWithSDK; assertions match new paths/payloads; errors via convertSDKError.
  • Bug Fixes

    • webhook list --all --page N now starts from page N when fetching all results; added a unit test to lock this in.

Written for commit e83f22c. Summary will update on new commits. Review in cubic

Copilot AI review requested due to automatic review settings April 29, 2026 20:34
@github-actions github-actions Bot added the enhancement New feature or request label Apr 29, 2026
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 2 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="internal/commands/webhook.go">

<violation number="1" location="internal/commands/webhook.go:59">
P2: `webhook list --all --page N` no longer honors the page flag, causing a behavior regression in pagination start point.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment thread internal/commands/webhook.go
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 29, 2026

⚠️ Potential breaking changes detected:

  • Removal of the 'printSuccessWithLocation' function will impact any outputs or scripts relying on the specific functionality it provided.
  • Modification of endpoint paths by removing '.json' suffixes (e.g., '/boards/board-1/webhooks/wh-1.json' changed to '/boards/board-1/webhooks/wh-1') may break scripts or integrations that expect the original URL format.
  • Changes to the output of various commands (e.g., switching from 'resp.Data' to normalized data via 'normalizeAny') could affect scripts parsing the data, as the structure or content might differ.
  • Replacement of manual API client methods (e.g., 'client.GetWithPagination' or 'client.Post') with SDK-generated methods (e.g., 'ac.Get' or 'ac.Webhooks().Activate') might alter behavior, particularly with error messages or returned data.

Review carefully before merging. Consider a major version bump.

Replaces the legacy HTTP client (getClient()) with the typed
WebhooksService for show/create/update/delete/reactivate. The list
command uses the typed service for the no-flag case, falling back to
ac.Get / ac.GetAll for --page and --all (the SDK's Webhooks().List
doesn't accept a path).

Behavior, flags, and CLI surface are unchanged. Unit tests updated to
the SetTestModeWithSDK pattern; assertions now match the SDK's request
shape (no `webhook` body wrapper) and URL paths (no .json suffix on
get/update/delete).
`webhook list --all --page N` previously dropped the page param when
--all was set, causing GetAll to start at page 1 instead of page N. The
legacy client's GetWithPagination preserved the start page when paging
through all results; the migration regressed this.

Fixed by appending the page param to the path before invoking GetAll.
Added a unit test to lock in the behavior.
The webhook migration removed the only call site (Webhooks().Create returns the full struct directly, no Location follow). Drop the dead helper to satisfy the unused-code lint check.
Copilot AI review requested due to automatic review settings April 30, 2026 12:53
@robzolkos robzolkos force-pushed the migrate-webhook-to-sdk branch from e83f22c to 52ac9f5 Compare April 30, 2026 12:53
@github-actions github-actions Bot added bug Something isn't working and removed enhancement New feature or request labels Apr 30, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/commands/webhook.go Outdated
Comment thread internal/commands/webhook_test.go Outdated
@github-actions github-actions Bot added enhancement New feature or request and removed bug Something isn't working labels Apr 30, 2026
@robzolkos robzolkos merged commit 6976c76 into master Apr 30, 2026
20 checks passed
@robzolkos robzolkos deleted the migrate-webhook-to-sdk branch April 30, 2026 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants