(DHQ-540) Add dry-run deployments and pagination control#2
Merged
facundofarias merged 5 commits intomainfrom Apr 17, 2026
Merged
(DHQ-540) Add dry-run deployments and pagination control#2facundofarias merged 5 commits intomainfrom
facundofarias merged 5 commits intomainfrom
Conversation
Add ListOptions struct with Page/PerPage fields and appendListParams helper to build query strings. All 24 List* methods now accept an optional *ListOptions parameter (nil preserves current behavior). Fix Pagination struct field names to match actual API response (total_records/offset instead of total_count/per_page). Update all callers to pass nil for the new parameter.
…ands Dry-run: dhq deploy --dry-run creates a preview deployment via the API (mode: "preview") without executing. Returns preview_pending status. Mutually exclusive with --wait. Pagination: --page and --per-page flags on deployments list, servers list, and server-groups list (the three endpoints that support server-side pagination). JSON output includes pagination metadata via new NewPaginatedResponse envelope. TTY output shows page footer when multiple pages exist.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds SDK list pagination support and CLI flags ( Changes
Sequence Diagram(s)mermaid User->>CLI: run Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
MartaKar
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/commands/servers.go (1)
34-83:⚠️ Potential issue | 🟠 Major
servers listapplies pagination inputs but drops pagination outputs.Line 50 passes page/per-page options into the SDK call, but Lines 57-73 still render a plain list response (
output.NewResponse) with no pagination metadata or page footer. This makes the pagination feature incomplete for this command’s JSON and TTY outputs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/servers.go` around lines 34 - 83, The List command passes page/perPage into listOptsFromFlags and client.ListServers but then drops pagination metadata when rendering; update the rendering to surface pagination for both JSON and TTY: have the code capture pagination info returned or obtainable (page, perPage, total/hasMore) and pass it into output.NewResponse (so JSON includes pagination fields) and append a footer via env.Status for TTY (e.g., "Page X of Y — use --page/--per-page" or next/prev hints). Modify the block using output.NewResponse, env.WriteJSON, env.WriteTable and env.Status accordingly and ensure addPaginationFlags, listOptsFromFlags, and ListServers usage remain consistent.
🧹 Nitpick comments (2)
internal/commands/test_access.go (1)
37-45: Server name resolution currently checks only the first page of servers.For accounts with many servers, a valid
--server <name>may fail to resolve if it’s not on page 1. Consider paging through results during resolution.♻️ Suggested fix
- servers, err := client.ListServers(cliCtx.Background(), projectID, nil) + servers, err := listAllServers(cliCtx.Background(), client, projectID)func listAllServers(ctx context.Context, client *sdk.Client, projectID string) ([]sdk.Server, error) { const perPage = 100 var all []sdk.Server for page := 1; ; page++ { chunk, err := client.ListServers(ctx, projectID, &sdk.ListOptions{ Page: page, PerPage: perPage, }) if err != nil { return nil, err } all = append(all, chunk...) if len(chunk) < perPage { break } } return all, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/test_access.go` around lines 37 - 45, The current server-name resolution in the test access command only calls client.ListServers once (in the block around resolveServerName) so names not on the first page are missed; change this to iterate pages (e.g., implement a helper like listAllServers that repeatedly calls client.ListServers with ListOptions.Page and PerPage until fewer than perPage results are returned) and use the combined slice when calling resolveServerName; update the code path that currently does client.ListServers(cliCtx.Background(), projectID, nil) to call the new paging helper and propagate any errors appropriately.internal/commands/completions_dynamic.go (1)
34-34: Server completions may be truncated to page 1.Using
niloptions against a paginated servers endpoint can hide valid completion entries on later pages.♻️ Suggested fix
- servers, err := client.ListServers(cliCtx.Background(), projectID, nil) + servers, err := listAllServers(cliCtx.Background(), client, projectID)Use the same paged helper approach as in other server-name resolution paths to return full completion candidates.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/completions_dynamic.go` at line 34, The call using client.ListServers(cliCtx.Background(), projectID, nil) can miss servers on subsequent pages; replace this single-page call with the paged helper used elsewhere for server-name resolution (iterate across pages and append results into the servers slice) so all completion candidates are returned — locate the ListServers usage in completions_dynamic.go and swap it for the same pagination logic/paged helper used by other server-name resolution code paths (collect into the existing servers variable using projectID and cliCtx.Background()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/commands/deploy_test.go`:
- Around line 17-22: The test fails early due to authentication checks, so make
it deterministic by ensuring the CLI is authenticated (or auth checks are
stubbed) before calling NewRootCmd("test"). Update the test to establish an
authenticated context—e.g., call the existing test helper or function that
creates a logged-in session, set the auth env/token used by the CLI, or mock the
auth layer—so that when cmd := NewRootCmd("test"); cmd.SetArgs(...); err :=
cmd.Execute() runs it reaches the mutual-exclusivity validation and returns the
expected error containing "mutually exclusive" from the deploy command logic.
In `@internal/commands/deploy.go`:
- Around line 189-190: The breadcrumb command builder deployExecuteCmd is
currently always appending "-s" and omits the resolved revision used for
preview, which can yield an invalid "-s" with empty value or run the wrong
revision; update the code that constructs the breadcrumb (where
output.Breadcrumb{Action: "execute", Cmd: deployExecuteCmd(...) } is used) to
pass the resolved revision string into deployExecuteCmd and change
deployExecuteCmd to only include the "-s <revision>" flag when the revision is
non-empty, ensuring the breadcrumb command exactly matches the
previewed/resolved revision; make the same change for the other occurrences of
deployExecuteCmd in the file (the second usage block).
In `@internal/commands/pagination_test.go`:
- Around line 24-27: The test currently only checks deploymentsCmd is non-nil
after calling cmd.Find, which can mask errors because cmd.Find returns a (cmd,
args, error); update the assertions to also assert that the error is nil for
each cmd.Find call (e.g., when locating "deployments list" and the other
occurrences at lines 32-35 and 40-43) before checking
deploymentsCmd.Flags().Lookup("page") and ("per-page"), so replace or add checks
using assert.NoError(t, err) (or require.NoError) for the returned error from
cmd.Find to avoid false positives.
In `@pkg/sdk/deployments.go`:
- Around line 48-49: The function currently returns &preview even when c.post
returns an error, which can expose a zero-value DeploymentPreview; update the
return logic around the c.post call so that if err from c.post (the call to
c.post(ctx, fmt.Sprintf("/projects/%s/deployments", projectID), body, &preview))
is non-nil you return nil, err, otherwise return &preview, nil—ensuring callers
receive a nil *DeploymentPreview when an API error occurs.
---
Outside diff comments:
In `@internal/commands/servers.go`:
- Around line 34-83: The List command passes page/perPage into listOptsFromFlags
and client.ListServers but then drops pagination metadata when rendering; update
the rendering to surface pagination for both JSON and TTY: have the code capture
pagination info returned or obtainable (page, perPage, total/hasMore) and pass
it into output.NewResponse (so JSON includes pagination fields) and append a
footer via env.Status for TTY (e.g., "Page X of Y — use --page/--per-page" or
next/prev hints). Modify the block using output.NewResponse, env.WriteJSON,
env.WriteTable and env.Status accordingly and ensure addPaginationFlags,
listOptsFromFlags, and ListServers usage remain consistent.
---
Nitpick comments:
In `@internal/commands/completions_dynamic.go`:
- Line 34: The call using client.ListServers(cliCtx.Background(), projectID,
nil) can miss servers on subsequent pages; replace this single-page call with
the paged helper used elsewhere for server-name resolution (iterate across pages
and append results into the servers slice) so all completion candidates are
returned — locate the ListServers usage in completions_dynamic.go and swap it
for the same pagination logic/paged helper used by other server-name resolution
code paths (collect into the existing servers variable using projectID and
cliCtx.Background()).
In `@internal/commands/test_access.go`:
- Around line 37-45: The current server-name resolution in the test access
command only calls client.ListServers once (in the block around
resolveServerName) so names not on the first page are missed; change this to
iterate pages (e.g., implement a helper like listAllServers that repeatedly
calls client.ListServers with ListOptions.Page and PerPage until fewer than
perPage results are returned) and use the combined slice when calling
resolveServerName; update the code path that currently does
client.ListServers(cliCtx.Background(), projectID, nil) to call the new paging
helper and propagate any errors appropriately.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d1271129-e56f-4588-8831-9a5173bd6b19
📒 Files selected for processing (67)
CLAUDE.mdinternal/assist/context.gointernal/commands/activity.gointernal/commands/auth.gointernal/commands/auto_deploys.gointernal/commands/build_commands.gointernal/commands/build_configs.gointernal/commands/completions_dynamic.gointernal/commands/config_files.gointernal/commands/configure.gointernal/commands/deploy.gointernal/commands/deploy_test.gointernal/commands/deployments.gointernal/commands/doctor.gointernal/commands/env_vars.gointernal/commands/excluded_files.gointernal/commands/global_servers.gointernal/commands/hello.gointernal/commands/init.gointernal/commands/language_versions.gointernal/commands/network_agents.gointernal/commands/pagination.gointernal/commands/pagination_test.gointernal/commands/projects.gointernal/commands/repos.gointernal/commands/scheduled_deploys.gointernal/commands/server_groups.gointernal/commands/servers.gointernal/commands/ssh_commands.gointernal/commands/ssh_keys.gointernal/commands/status.gointernal/commands/templates.gointernal/commands/test_access.gointernal/commands/url.gointernal/commands/zones.gointernal/output/breadcrumbs.gointernal/output/breadcrumbs_test.gopkg/sdk/activity.gopkg/sdk/agents.gopkg/sdk/auto_deployments.gopkg/sdk/build_commands.gopkg/sdk/build_configs.gopkg/sdk/client.gopkg/sdk/client_test.gopkg/sdk/config_files.gopkg/sdk/deployments.gopkg/sdk/deployments_test.gopkg/sdk/env_vars.gopkg/sdk/excluded_files.gopkg/sdk/global_servers.gopkg/sdk/integrations.gopkg/sdk/language_versions.gopkg/sdk/projects.gopkg/sdk/projects_test.gopkg/sdk/repositories.gopkg/sdk/repositories_test.gopkg/sdk/scheduled_deployments.gopkg/sdk/server_groups.gopkg/sdk/server_groups_test.gopkg/sdk/servers.gopkg/sdk/servers_test.gopkg/sdk/ssh_commands.gopkg/sdk/ssh_keys.gopkg/sdk/templates.gopkg/sdk/templates_test.gopkg/sdk/types.gopkg/sdk/zones.go
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
The mutual exclusivity check was after cliCtx.Client() which requires auth. In CI (no keyring), the test hit "Not logged in" before reaching the flag validation. Move the check earlier.
- Return nil preview on API error in PreviewDeployment (avoid zero-value use) - Use require.NoError on cmd.Find in pagination flag tests (stricter assertions)
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/commands/deploy.go (1)
186-190:⚠️ Potential issue | 🟠 MajorFix execute breadcrumb to preserve preview context and avoid invalid
-s.The generated execute command can be wrong: it always emits
-s(even when empty) and omits the resolvedrevision, so it may execute a different commit than the previewed one.💡 Proposed fix
- output.Breadcrumb{Action: "execute", Cmd: deployExecuteCmd(projectID, server, branch)}, + output.Breadcrumb{Action: "execute", Cmd: deployExecuteCmd(projectID, server, branch, revision)},-func deployExecuteCmd(projectID, server, branch string) string { - cmd := fmt.Sprintf("dhq deploy -p %s -s %s", projectID, server) +func deployExecuteCmd(projectID, server, branch, revision string) string { + cmd := fmt.Sprintf("dhq deploy -p %s", projectID) + if server != "" { + cmd += " -s " + server + } if branch != "" { cmd += " -b " + branch } + if revision != "" { + cmd += " -r " + revision + } return cmd }Also applies to: 254-260
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/deploy.go` around lines 186 - 190, The execute breadcrumb is built with deployExecuteCmd(projectID, server, branch) which always emits a "-s" flag even when service is empty and omits the preview's resolved revision; update the breadcrumb construction to call deployExecuteCmd with the preview's resolved revision and only include the service flag when preview.Service (or the equivalent preview.ServiceName) is non-empty so "-s" is not emitted as an empty flag; change the env.WriteJSON/new output.Breadcrumb usage around preview handling (references: preview.Identifier, preview.Status, preview.Revision, preview.Service and deployExecuteCmd) and apply the same fix to the other JSON/TTY block that builds the execute breadcrumb.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/commands/deploy.go`:
- Around line 186-190: The execute breadcrumb is built with
deployExecuteCmd(projectID, server, branch) which always emits a "-s" flag even
when service is empty and omits the preview's resolved revision; update the
breadcrumb construction to call deployExecuteCmd with the preview's resolved
revision and only include the service flag when preview.Service (or the
equivalent preview.ServiceName) is non-empty so "-s" is not emitted as an empty
flag; change the env.WriteJSON/new output.Breadcrumb usage around preview
handling (references: preview.Identifier, preview.Status, preview.Revision,
preview.Service and deployExecuteCmd) and apply the same fix to the other
JSON/TTY block that builds the execute breadcrumb.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f7ce6195-5f68-4779-998e-14aa551ac787
📒 Files selected for processing (1)
internal/commands/deploy.go
facundofarias
approved these changes
Apr 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Linear: https://linear.app/deployhq/issue/DHQ-540/cli-add-dry-run-and-pagination-support-api-dependent
Summary
--dry-runondhq deploy: Creates a preview deployment via the API (mode: "preview") without executing. Returnspreview_pendingstatus. Mutually exclusive with--wait.--page/--per-pageon paginated list commands: Added todeployments list,servers list, andserver-groups list(the three endpoints with server-side pagination). JSON output includespaginationmetadata.ListOptions: All 24List*methods now accept*ListOptionsfor pagination params. Passingnilpreserves current behavior.total_records/offsetinstead oftotal_count/per_page).Tested against staging
deployments list --page 1 --jsontotal_records: 5)servers list --page 1deploy --dry-run --json{"status": "preview_pending", "identifier": "..."}deploy --dry-run --waitdeploy --dry-run(TTY)Test plan
dhq deploy --dry-run -p <project> -s <server>creates preview without deployingdhq deployments list -p <project> --page 1 --jsonincludes pagination metadatadhq servers list -p <project> --page 1 --per-page 1limits resultsdhq deploy --dry-run --waiterrors with mutual exclusivity messagego test ./...— all 102 tests pass