From 04a4555277715c5cac335a10bae310c7e4cff0fd Mon Sep 17 00:00:00 2001 From: Facundo Farias Date: Mon, 1 Jun 2026 09:14:33 +0200 Subject: [PATCH 1/2] fix(deploy): use server group's last_revision for incremental deploys MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deploying to a server group via `dhq deploy -s "Group Name"` always performed a full deploy from the first commit, ignoring the group's prior deployment history. When the `-s` value resolved to a group, the code set `resolvedServer = nil` and never captured the group, so `resolveStartRevision` (which only looked at `Server.LastRevision`) returned `""`. The API treats an empty `start_revision` as "deploy entire branch from the first commit" — exactly the fresh-deploy behaviour the customer reported. Capture `resolvedGroup *sdk.ServerGroup` alongside `resolvedServer` so both name- and UUID-based group resolution flow into the same incremental path used for individual servers. `ServerGroup.LastRevision` is the `end_revision` of the most recent group-targeted deployment (Rails: `ServerGroup#last_revision`), which is the correct incremental baseline. Newly-created groups have an empty value so the first group deploy still falls through to a full deploy. Also use `ServerGroup.PreferredBranch` when no server is resolved, so group deploys honour the group's configured branch instead of silently deploying the repository default branch's tip. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/commands/deploy.go | 73 +++++++----- internal/commands/deploy_test.go | 183 ++++++++++++++++++++++++++----- 2 files changed, 202 insertions(+), 54 deletions(-) diff --git a/internal/commands/deploy.go b/internal/commands/deploy.go index 9402238..1c4bc48 100644 --- a/internal/commands/deploy.go +++ b/internal/commands/deploy.go @@ -19,7 +19,8 @@ func isUUID(s string) bool { // resolveBranchAndRevision determines the branch and end revision for a deployment. // -// Branch resolution: explicit --branch → server's PreferredBranch (or Branch) → "" (use repo default). +// Branch resolution: explicit --branch → server's PreferredBranch (or Branch) → group's +// PreferredBranch → "" (use repo default). // Revision resolution: explicit --revision → tip SHA of the resolved branch → repo default tip. // // Sending end_revision without ensuring it matches the chosen branch was the source of two @@ -30,25 +31,29 @@ func resolveBranchAndRevision( client *sdk.Client, projectID, serverIdentifier, flagBranch, flagRevision string, knownServer *sdk.Server, + knownGroup *sdk.ServerGroup, ) (branch, revision string, err error) { branch = flagBranch revision = flagRevision if branch == "" && serverIdentifier != "" { srv := knownServer - if srv == nil { - // GetServer 404s for server-group identifiers; that's expected — we fall through. + grp := knownGroup + if srv == nil && grp == nil { + // GetServer 404s for server-group identifiers; fall through to GetServerGroup. if s, gerr := client.GetServer(ctx, projectID, serverIdentifier); gerr == nil { srv = s + } else if g, gerr := client.GetServerGroup(ctx, projectID, serverIdentifier); gerr == nil { + grp = g } } - if srv != nil { - switch { - case srv.PreferredBranch != "": - branch = srv.PreferredBranch - case srv.Branch != "": - branch = srv.Branch - } + switch { + case srv != nil && srv.PreferredBranch != "": + branch = srv.PreferredBranch + case srv != nil && srv.Branch != "": + branch = srv.Branch + case grp != nil && grp.PreferredBranch != "": + branch = grp.PreferredBranch } } @@ -81,13 +86,16 @@ func resolveBranchAndRevision( // resolveStartRevision determines the start_revision for a deployment. // // Resolution order: --full → "" (deploy entire branch) → explicit --start-revision → -// resolved server's LastRevision (incremental from last successful deploy) → "". +// resolved server's LastRevision → resolved group's LastRevision → "". // // Sending an empty start_revision to the API means "deploy entire branch from the -// first commit". That was the source of issue #5: dhq deploy never populated this -// field, so every deploy looked like an initial one regardless of what the server -// had previously deployed. -func resolveStartRevision(server *sdk.Server, flagStart string, full bool) string { +// first commit". That was the source of issue #5 (server case) and the DHQ-586 +// follow-up (group case): without populating this field every deploy looked like +// an initial one. ServerGroup.LastRevision is the end_revision of the most recent +// deployment that targeted the group (Rails: ServerGroup#last_revision), which is +// the correct incremental baseline. Newly-created groups have an empty value, so +// the first group deploy still falls through to a full deploy. +func resolveStartRevision(server *sdk.Server, group *sdk.ServerGroup, flagStart string, full bool) string { if full { return "" } @@ -97,6 +105,9 @@ func resolveStartRevision(server *sdk.Server, flagStart string, full bool) strin if server != nil && server.LastRevision != "" { return server.LastRevision } + if group != nil && group.LastRevision != "" { + return group.LastRevision + } return "" } @@ -274,9 +285,11 @@ func newDeployCmd() *cobra.Command { env := cliCtx.Envelope - // Track the Server we resolved to, so branch/revision lookup can reuse it - // without an extra GetServer round-trip. + // Track the Server or ServerGroup we resolved to, so branch/revision lookup + // can reuse them without extra round-trips. Exactly one of these will be + // non-nil once the target is locked in (or both nil for a project-wide deploy). var resolvedServer *sdk.Server + var resolvedGroup *sdk.ServerGroup // Auto-select server if not specified if server == "" { @@ -337,10 +350,16 @@ func newDeployCmd() *cobra.Command { // (documented at deployhq.com/support/cli/cli-deploying). if !matched { if groups, gerr := client.ListServerGroups(cliCtx.Background(), projectID, nil); gerr == nil { - if groupID, groupName := resolveGroupName(server, groups); groupID != "" { + if groupID, _ := resolveGroupName(server, groups); groupID != "" { + for i := range groups { + if groups[i].Identifier == groupID { + resolvedGroup = &groups[i] + break + } + } server = groupID resolvedServer = nil // groups don't map to a single Server - env.Status("Resolved to server group: %s", groupName) + env.Status("Resolved to server group: %s", resolvedGroup.Name) matched = true } } @@ -375,20 +394,22 @@ func newDeployCmd() *cobra.Command { } } - // Eagerly fetch the server when caller didn't already resolve it - // (e.g. user passed a UUID directly). We need its LastRevision for - // start_revision resolution. GetServer 404s for server-group - // identifiers — that's expected, we just leave resolvedServer nil. - if resolvedServer == nil && server != "" { + // Eagerly fetch the target when caller didn't already resolve it + // (e.g. user passed a UUID directly). We need either Server.LastRevision or + // ServerGroup.LastRevision for start_revision resolution. GetServer 404s for + // server-group identifiers, so fall back to GetServerGroup before giving up. + if resolvedServer == nil && resolvedGroup == nil && server != "" { if s, gerr := client.GetServer(cliCtx.Background(), projectID, server); gerr == nil { resolvedServer = s + } else if g, gerr := client.GetServerGroup(cliCtx.Background(), projectID, server); gerr == nil { + resolvedGroup = g } } if branch == "" || revision == "" { env.Status("Resolving branch and revision...") resolvedBranch, resolvedRev, err := resolveBranchAndRevision( - cliCtx.Background(), client, projectID, server, branch, revision, resolvedServer, + cliCtx.Background(), client, projectID, server, branch, revision, resolvedServer, resolvedGroup, ) if err != nil { return err @@ -398,7 +419,7 @@ func newDeployCmd() *cobra.Command { } req := sdk.DeploymentCreateRequest{ - StartRevision: resolveStartRevision(resolvedServer, startRevision, full), + StartRevision: resolveStartRevision(resolvedServer, resolvedGroup, startRevision, full), Branch: branch, EndRevision: revision, ParentIdentifier: server, diff --git a/internal/commands/deploy_test.go b/internal/commands/deploy_test.go index b8258a1..43f231f 100644 --- a/internal/commands/deploy_test.go +++ b/internal/commands/deploy_test.go @@ -37,15 +37,18 @@ func newTestSDKClient(t *testing.T, server *httptest.Server) *sdk.Client { return c } -// branchEndpointMux serves /branches and /servers/:id and /latest_revision. -// Tracks which endpoints were called so tests can verify the resolution order. +// branchEndpointMux serves /branches and /servers/:id and /latest_revision and +// /server_groups/:id. Tracks which endpoints were called so tests can verify the +// resolution order. type branchEndpointMux struct { - branches map[string]string - servers map[string]sdk.Server - latestRevision string - branchesCalled bool - latestRevCalled bool - getServerCalledWith string + branches map[string]string + servers map[string]sdk.Server + groups map[string]sdk.ServerGroup + latestRevision string + branchesCalled bool + latestRevCalled bool + getServerCalledWith string + getServerGroupCalledWith string } func (m *branchEndpointMux) handler(t *testing.T) http.HandlerFunc { @@ -69,6 +72,17 @@ func (m *branchEndpointMux) handler(t *testing.T) http.HandlerFunc { return } _ = json.NewEncoder(w).Encode(s) + case r.Method == http.MethodGet && len(r.URL.Path) > len("/projects/p/server_groups/") && + r.URL.Path[:len("/projects/p/server_groups/")] == "/projects/p/server_groups/": + id := r.URL.Path[len("/projects/p/server_groups/"):] + m.getServerGroupCalledWith = id + g, ok := m.groups[id] + if !ok { + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"error":"not found"}`)) + return + } + _ = json.NewEncoder(w).Encode(g) default: t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) w.WriteHeader(http.StatusInternalServerError) @@ -96,7 +110,7 @@ func TestResolveBranchAndRevision_UsesServerPreferredBranch(t *testing.T) { } branch, revision, err := resolveBranchAndRevision( - t.Context(), client, "p", "srv-1", "", "", knownServer, + t.Context(), client, "p", "srv-1", "", "", knownServer, nil, ) require.NoError(t, err) assert.Equal(t, "develop", branch, "should use server's preferred_branch") @@ -126,7 +140,7 @@ func TestResolveBranchAndRevision_FlagBranchOverridesServerPreferred(t *testing. } branch, revision, err := resolveBranchAndRevision( - t.Context(), client, "p", "srv-1", "feature-x", "", knownServer, + t.Context(), client, "p", "srv-1", "feature-x", "", knownServer, nil, ) require.NoError(t, err) assert.Equal(t, "feature-x", branch) @@ -148,7 +162,7 @@ func TestResolveBranchAndRevision_FetchesServerWhenNotKnown(t *testing.T) { client := newTestSDKClient(t, srv) branch, revision, err := resolveBranchAndRevision( - t.Context(), client, "p", "srv-uuid", "", "", nil, // knownServer = nil + t.Context(), client, "p", "srv-uuid", "", "", nil, nil, // knownServer = nil, knownGroup = nil ) require.NoError(t, err) assert.Equal(t, "prod-branch", branch) @@ -168,7 +182,7 @@ func TestResolveBranchAndRevision_ServerGroupFallsBackGracefully(t *testing.T) { client := newTestSDKClient(t, srv) branch, revision, err := resolveBranchAndRevision( - t.Context(), client, "p", "group-id", "", "", nil, + t.Context(), client, "p", "group-id", "", "", nil, nil, ) require.NoError(t, err) assert.Equal(t, "", branch, "no branch when server-group has no preferred branch") @@ -187,7 +201,7 @@ func TestResolveBranchAndRevision_ExplicitRevisionRespected(t *testing.T) { knownServer := &sdk.Server{Identifier: "srv-1", PreferredBranch: "main"} branch, revision, err := resolveBranchAndRevision( - t.Context(), client, "p", "srv-1", "feature-x", "explicit-sha", knownServer, + t.Context(), client, "p", "srv-1", "feature-x", "explicit-sha", knownServer, nil, ) require.NoError(t, err) assert.Equal(t, "feature-x", branch) @@ -207,7 +221,7 @@ func TestResolveBranchAndRevision_UnknownBranchErrors(t *testing.T) { client := newTestSDKClient(t, srv) _, _, err := resolveBranchAndRevision( - t.Context(), client, "p", "", "nope", "", nil, + t.Context(), client, "p", "", "nope", "", nil, nil, ) require.Error(t, err) assert.Contains(t, err.Error(), `"nope"`) @@ -222,7 +236,7 @@ func TestResolveBranchAndRevision_FallsBackWhenNoBranchAvailable(t *testing.T) { client := newTestSDKClient(t, srv) branch, revision, err := resolveBranchAndRevision( - t.Context(), client, "p", "", "", "", nil, + t.Context(), client, "p", "", "", "", nil, nil, ) require.NoError(t, err) assert.Equal(t, "", branch) @@ -235,7 +249,7 @@ func TestResolveStartRevision_DefaultsToServerLastRevision(t *testing.T) { // never populated start_revision. The default for an incremental deploy is // the server's last successfully deployed commit. srv := &sdk.Server{Identifier: "srv-1", LastRevision: "last-deploy-sha"} - got := resolveStartRevision(srv, "", false) + got := resolveStartRevision(srv, nil, "", false) assert.Equal(t, "last-deploy-sha", got) } @@ -243,7 +257,7 @@ func TestResolveStartRevision_FlagOverridesServerLastRevision(t *testing.T) { // --start-revision is the explicit override path (e.g. for hotfixes // starting from a specific commit other than the last deploy). srv := &sdk.Server{Identifier: "srv-1", LastRevision: "last-deploy-sha"} - got := resolveStartRevision(srv, "explicit-start", false) + got := resolveStartRevision(srv, nil, "explicit-start", false) assert.Equal(t, "explicit-start", got) } @@ -251,7 +265,7 @@ func TestResolveStartRevision_FullForcesEmpty(t *testing.T) { // --full means "deploy entire branch from first commit" — the API treats // an empty start_revision as that signal. srv := &sdk.Server{Identifier: "srv-1", LastRevision: "last-deploy-sha"} - got := resolveStartRevision(srv, "", true) + got := resolveStartRevision(srv, nil, "", true) assert.Equal(t, "", got, "--full must clear start_revision even when server has a last_revision") } @@ -259,20 +273,20 @@ func TestResolveStartRevision_FullBeatsExplicitFlag(t *testing.T) { // Defensive: even if both somehow get through (the cobra-level mutual // exclusivity check should catch it), --full wins. srv := &sdk.Server{Identifier: "srv-1", LastRevision: "last-deploy-sha"} - got := resolveStartRevision(srv, "explicit-start", true) + got := resolveStartRevision(srv, nil, "explicit-start", true) assert.Equal(t, "", got) } -func TestResolveStartRevision_NilServerReturnsEmpty(t *testing.T) { - // Server-group deploys and project-wide deploys have no single Server to - // read from — fall back to "" and let the API decide per server. - got := resolveStartRevision(nil, "", false) +func TestResolveStartRevision_NilTargetsReturnEmpty(t *testing.T) { + // Project-wide deploys have no Server or ServerGroup to read from — fall + // back to "" and let the API decide per server. + got := resolveStartRevision(nil, nil, "", false) assert.Equal(t, "", got) } -func TestResolveStartRevision_NilServerStillRespectsFlag(t *testing.T) { - // Even without a resolved server, an explicit --start-revision must be honored. - got := resolveStartRevision(nil, "explicit-start", false) +func TestResolveStartRevision_NilTargetsStillRespectFlag(t *testing.T) { + // Even without a resolved target, an explicit --start-revision must be honored. + got := resolveStartRevision(nil, nil, "explicit-start", false) assert.Equal(t, "explicit-start", got) } @@ -280,7 +294,43 @@ func TestResolveStartRevision_FreshServerReturnsEmpty(t *testing.T) { // A server that has never been deployed has LastRevision="". First deploy // must be a full one — there's no incremental baseline to start from. srv := &sdk.Server{Identifier: "srv-1", LastRevision: ""} - got := resolveStartRevision(srv, "", false) + got := resolveStartRevision(srv, nil, "", false) + assert.Equal(t, "", got) +} + +func TestResolveStartRevision_GroupLastRevisionUsedWhenNoServer(t *testing.T) { + // DHQ-586 follow-up: deploying to a server group must be incremental from + // the group's last deploy. Without this, `dhq deploy -s "My Group"` always + // did a full deploy from the first commit even when the group had a deploy + // history. ServerGroup#last_revision (Rails) returns the end_revision of + // the group's most recent deployment, which is exactly what we want. + group := &sdk.ServerGroup{Identifier: "grp-1", LastRevision: "group-last-sha"} + got := resolveStartRevision(nil, group, "", false) + assert.Equal(t, "group-last-sha", got) +} + +func TestResolveStartRevision_GroupIgnoredWhenServerHasLastRevision(t *testing.T) { + // When both a server and group are present (single-server resolution), + // the server's LastRevision takes precedence. Groups only matter when the + // target is the group itself (resolvedServer == nil). + srv := &sdk.Server{Identifier: "srv-1", LastRevision: "server-last-sha"} + group := &sdk.ServerGroup{Identifier: "grp-1", LastRevision: "group-last-sha"} + got := resolveStartRevision(srv, group, "", false) + assert.Equal(t, "server-last-sha", got) +} + +func TestResolveStartRevision_FreshGroupReturnsEmpty(t *testing.T) { + // A newly-created group has no deployment history → LastRevision="" → + // first group deploy is correctly treated as a full deploy. + group := &sdk.ServerGroup{Identifier: "grp-1", LastRevision: ""} + got := resolveStartRevision(nil, group, "", false) + assert.Equal(t, "", got) +} + +func TestResolveStartRevision_FullClearsGroupLastRevision(t *testing.T) { + // --full overrides incremental for group deploys too. + group := &sdk.ServerGroup{Identifier: "grp-1", LastRevision: "group-last-sha"} + got := resolveStartRevision(nil, group, "", true) assert.Equal(t, "", got) } @@ -369,9 +419,86 @@ func TestResolveBranchAndRevision_PreferredBranchEmptyFallsToBranchField(t *test Branch: "legacy", // PreferredBranch left empty } branch, revision, err := resolveBranchAndRevision( - t.Context(), client, "p", "srv-1", "", "", knownServer, + t.Context(), client, "p", "srv-1", "", "", knownServer, nil, ) require.NoError(t, err) assert.Equal(t, "legacy", branch) assert.Equal(t, "sha-of-legacy", revision) } + +func TestResolveBranchAndRevision_UsesGroupPreferredBranch(t *testing.T) { + // DHQ-586 follow-up: when -s resolves to a server group, the group's + // preferred_branch must drive the deploy — otherwise the API gets + // end_revision from /repository/latest_revision (the default branch's tip), + // which can mismatch the group's intended branch. + mux := &branchEndpointMux{ + branches: map[string]string{ + "main": "sha-of-main", + "release": "sha-of-release", + }, + latestRevision: "sha-of-main", + } + srv := httptest.NewServer(mux.handler(t)) + defer srv.Close() + client := newTestSDKClient(t, srv) + + knownGroup := &sdk.ServerGroup{Identifier: "grp-1", PreferredBranch: "release"} + + branch, revision, err := resolveBranchAndRevision( + t.Context(), client, "p", "grp-1", "", "", nil, knownGroup, + ) + require.NoError(t, err) + assert.Equal(t, "release", branch, "group's preferred_branch must be honored") + assert.Equal(t, "sha-of-release", revision, "must be release's tip, not main's") + assert.False(t, mux.latestRevCalled, "should NOT fall back to repo default") +} + +func TestResolveBranchAndRevision_FetchesGroupWhenServerFails(t *testing.T) { + // When the caller didn't pre-resolve the target and the identifier is a + // group's UUID, GetServer 404s. We must fall through to GetServerGroup + // rather than silently dropping to the repo default branch. + mux := &branchEndpointMux{ + branches: map[string]string{"staging": "sha-of-staging"}, + servers: map[string]sdk.Server{}, // GetServer → 404 + groups: map[string]sdk.ServerGroup{ + "grp-uuid": {Identifier: "grp-uuid", PreferredBranch: "staging"}, + }, + } + srv := httptest.NewServer(mux.handler(t)) + defer srv.Close() + client := newTestSDKClient(t, srv) + + branch, revision, err := resolveBranchAndRevision( + t.Context(), client, "p", "grp-uuid", "", "", nil, nil, + ) + require.NoError(t, err) + assert.Equal(t, "staging", branch) + assert.Equal(t, "sha-of-staging", revision) + assert.Equal(t, "grp-uuid", mux.getServerCalledWith, "should attempt GetServer first") + assert.Equal(t, "grp-uuid", mux.getServerGroupCalledWith, "should fall through to GetServerGroup") +} + +func TestResolveBranchAndRevision_ServerPreferredBranchBeatsGroup(t *testing.T) { + // When both a server and a group surface a preferred_branch (e.g. the + // caller resolved a single-server target but also fetched its group), + // the server's setting wins because it's the more specific target. + mux := &branchEndpointMux{ + branches: map[string]string{ + "server-branch": "sha-of-server", + "group-branch": "sha-of-group", + }, + } + srv := httptest.NewServer(mux.handler(t)) + defer srv.Close() + client := newTestSDKClient(t, srv) + + knownServer := &sdk.Server{Identifier: "srv-1", PreferredBranch: "server-branch"} + knownGroup := &sdk.ServerGroup{Identifier: "grp-1", PreferredBranch: "group-branch"} + + branch, revision, err := resolveBranchAndRevision( + t.Context(), client, "p", "srv-1", "", "", knownServer, knownGroup, + ) + require.NoError(t, err) + assert.Equal(t, "server-branch", branch) + assert.Equal(t, "sha-of-server", revision) +} From 2a509c639aeba19cca8c7ea689e9bff9c298596e Mon Sep 17 00:00:00 2001 From: Facundo Farias Date: Mon, 1 Jun 2026 09:34:03 +0200 Subject: [PATCH 2/2] test(deploy): assert /branches is queried in group preferred-branch test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mirrors the equivalent assertion in TestResolveBranchAndRevision_UsesServerPreferredBranch so the test explicitly verifies the branch→sha lookup path was taken, not only that the repo-default fallback was skipped. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/commands/deploy_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/commands/deploy_test.go b/internal/commands/deploy_test.go index 43f231f..4a0872b 100644 --- a/internal/commands/deploy_test.go +++ b/internal/commands/deploy_test.go @@ -450,6 +450,7 @@ func TestResolveBranchAndRevision_UsesGroupPreferredBranch(t *testing.T) { require.NoError(t, err) assert.Equal(t, "release", branch, "group's preferred_branch must be honored") assert.Equal(t, "sha-of-release", revision, "must be release's tip, not main's") + assert.True(t, mux.branchesCalled, "should query /branches to map branch→sha") assert.False(t, mux.latestRevCalled, "should NOT fall back to repo default") }