From 20bafdd0a775342bc534b3ac089f3918e10fbb02 Mon Sep 17 00:00:00 2001 From: Olof Mattsson Date: Tue, 23 Jun 2026 13:02:32 +0200 Subject: [PATCH 1/3] fix(definition): update-chart preserves all fields + add --repository-url MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `stackctl definition update-chart` was silently wiping `repository_url`, `chart_path`, `build_pipeline_id`, and `deploy_order` on every call because the request body type (`UpdateChartConfigRequest`) only carried 4 of the 8 fields the backend stores, and the command's request builder seeded only 4 fields from `current` (the GET response). The backend's PUT handler replaced all fields with what was sent, so missing fields became "" on the server. Repro (against k8s-stack-manager < the companion fix): stackctl definition update-chart --chart-version 0.1.2 — before: repository_url=oci://acr/helm, chart_path=kvk-x, deploy_order=8 — after: repository_url="", chart_path="", deploy_order=0 Subsequent stack deploys then failed with helm's `non-absolute URLs should be in form of repo_name/path_to_chart` because there was no repository to fetch from. Changes: - `pkg/types`: `UpdateChartConfigRequest` now includes every field the backend's ChartConfig stores: `repository_url`, `build_pipeline_id`, + the ones it already had. All `omitempty` so an absent flag stays out of the JSON body when paired with a merge-safe backend. - `cmd/definition`: `update-chart` seeds `req` from EVERY field on the `current` GetDefinitionChart response (including `RepoURL`, `ChartPath`, `BuildPipelineID`, `DeployOrder`). User flags then override individual fields. `DeployOrder` is always non-nil because the backend's int is zero-valid (deploy_order=0 is meaningful). - Adds `--repository-url` flag. Without this, an empty `repository_url` that the user inherited from a prior bug (or wants to set explicitly) has no path to be set via stackctl. - Updates the at-least-one-flag validation to include the new flag. The companion k8s-stack-manager PR makes the backend merge-safe so old stackctl clients (which still send a partial body) stop wiping fields. Both fixes are needed long-term — backend safety against any client, plus stackctl carrying the full record so PUTs are explicit. Co-Authored-By: Claude Opus 4.7 (1M context) --- cli/cmd/definition.go | 26 ++++++++++++++++++++------ cli/pkg/types/types.go | 19 +++++++++++++------ 2 files changed, 33 insertions(+), 12 deletions(-) diff --git a/cli/cmd/definition.go b/cli/cmd/definition.go index 2bb041c..b810e04 100644 --- a/cli/cmd/definition.go +++ b/cli/cmd/definition.go @@ -402,11 +402,12 @@ Examples: chartPath, _ := cmd.Flags().GetString("chart-path") chartVersion, _ := cmd.Flags().GetString("chart-version") sourceRepoURL, _ := cmd.Flags().GetString("source-repo-url") + repositoryURL, _ := cmd.Flags().GetString("repository-url") deployOrder, _ := cmd.Flags().GetInt("deploy-order") valuesFile, _ := cmd.Flags().GetString("file") - if chartPath == "" && chartVersion == "" && sourceRepoURL == "" && deployOrder < 0 && valuesFile == "" { - return fmt.Errorf("at least one of --chart-path, --chart-version, --source-repo-url, --deploy-order, or --file must be specified") + if chartPath == "" && chartVersion == "" && sourceRepoURL == "" && repositoryURL == "" && deployOrder < 0 && valuesFile == "" { + return fmt.Errorf("at least one of --chart-path, --chart-version, --source-repo-url, --repository-url, --deploy-order, or --file must be specified") } if valuesFile != "" { @@ -427,11 +428,20 @@ Examples: return fmt.Errorf("fetching current chart config: %w", err) } + // Seed every field from the current record so a PUT round-trip + // doesn't wipe values the user didn't intend to change. Backend + // merges absent-from-JSON keys as "preserve"; we still send the + // full record because older backends do full PUT replace. + deployOrderCopy := current.DeployOrder req := types.UpdateChartConfigRequest{ - ChartName: current.ChartName, - ChartVersion: current.ChartVersion, - SourceRepoURL: current.SourceRepoURL, - DefaultValues: current.DefaultValues, + ChartName: current.ChartName, + RepositoryURL: current.RepoURL, + ChartPath: current.ChartPath, + ChartVersion: current.ChartVersion, + SourceRepoURL: current.SourceRepoURL, + BuildPipelineID: current.BuildPipelineID, + DeployOrder: &deployOrderCopy, + DefaultValues: current.DefaultValues, } if chartPath != "" { @@ -443,6 +453,9 @@ Examples: if sourceRepoURL != "" { req.SourceRepoURL = sourceRepoURL } + if repositoryURL != "" { + req.RepositoryURL = repositoryURL + } if deployOrder >= 0 { req.DeployOrder = &deployOrder } @@ -539,6 +552,7 @@ func init() { definitionUpdateChartCmd.Flags().String("chart-path", "", "Chart path (e.g. /charts/kvk-core)") definitionUpdateChartCmd.Flags().String("chart-version", "", "Chart version") definitionUpdateChartCmd.Flags().String("source-repo-url", "", "Git repository URL for branch listing") + definitionUpdateChartCmd.Flags().String("repository-url", "", "Helm chart repository URL (e.g. oci://acr.example.com/helm)") definitionUpdateChartCmd.Flags().Int("deploy-order", -1, "Deploy order (0+)") definitionUpdateChartCmd.Flags().String("file", "", "File containing default values") diff --git a/cli/pkg/types/types.go b/cli/pkg/types/types.go index c1eead5..2c50d81 100644 --- a/cli/pkg/types/types.go +++ b/cli/pkg/types/types.go @@ -629,13 +629,20 @@ type UpdateDefinitionRequest struct { } // UpdateChartConfigRequest is the request body for PUT /api/v1/stack-definitions/:id/charts/:chartID. +// +// Includes every field the backend's ChartConfig knows about, so a +// GET-then-PUT round-trip in update-chart doesn't silently wipe fields +// stackctl flags don't expose. Backend treats absent JSON keys as +// "preserve existing"; explicit "" replaces. type UpdateChartConfigRequest struct { - ChartName string `json:"chart_name,omitempty" yaml:"chart_name,omitempty"` - ChartPath string `json:"chart_path,omitempty" yaml:"chart_path,omitempty"` - ChartVersion string `json:"chart_version,omitempty" yaml:"chart_version,omitempty"` - SourceRepoURL string `json:"source_repo_url,omitempty" yaml:"source_repo_url,omitempty"` - DeployOrder *int `json:"deploy_order,omitempty" yaml:"deploy_order,omitempty"` - DefaultValues string `json:"default_values,omitempty" yaml:"default_values,omitempty"` + ChartName string `json:"chart_name,omitempty" yaml:"chart_name,omitempty"` + RepositoryURL string `json:"repository_url,omitempty" yaml:"repository_url,omitempty"` + ChartPath string `json:"chart_path,omitempty" yaml:"chart_path,omitempty"` + ChartVersion string `json:"chart_version,omitempty" yaml:"chart_version,omitempty"` + SourceRepoURL string `json:"source_repo_url,omitempty" yaml:"source_repo_url,omitempty"` + BuildPipelineID string `json:"build_pipeline_id,omitempty" yaml:"build_pipeline_id,omitempty"` + DeployOrder *int `json:"deploy_order,omitempty" yaml:"deploy_order,omitempty"` + DefaultValues string `json:"default_values,omitempty" yaml:"default_values,omitempty"` } // OrphanedNamespace represents a Kubernetes namespace with no matching stack record. From b03a50f4482188a9a8098ea52e284a10f6fdb6d0 Mon Sep 17 00:00:00 2001 From: Olof Mattsson Date: Tue, 23 Jun 2026 17:17:37 +0200 Subject: [PATCH 2/3] fix(definition): validate --repository-url, harden update-chart tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address adversarial-review findings on the update-chart change: - Add scheme allowlist (oci/http/https) for --repository-url; reject empty Host (catches `oci:/x` typo) and unsupported schemes. - Drop the misleading "explicit '' replaces" docstring on UpdateChartConfigRequest — every string field has omitempty so empty values are dropped from the wire and the backend preserves; there is no clear-field path through this request. - Inline ¤t.DeployOrder (drop the deployOrderCopy indirection that implied a non-existent loop-capture concern). - Document --repository-url in the cobra Long examples block. - Extend sampleChartConfig with ChartPath, SourceRepoURL, BuildPipelineID, DeployOrder so PUT round-trip assertions can verify the seeded fields actually reach the wire. - Assert seeded fields round-trip in the existing ChartVersion test — catches regressions of the original wipe bug. - Add TestDefinitionUpdateChartCmd_RepositoryURL (happy path) and TestDefinitionUpdateChartCmd_RepositoryURL_Invalid (rejection of not-a-url, oci:/single-slash, ftp scheme). - Reset source-repo-url and repository-url in every t.Cleanup — closes the package-global flag-state leak that this PR widened. Co-Authored-By: Claude Opus 4.7 (1M context) --- cli/cmd/definition.go | 17 ++++++- cli/cmd/definition_test.go | 100 ++++++++++++++++++++++++++++++++++--- cli/pkg/types/types.go | 5 +- 3 files changed, 111 insertions(+), 11 deletions(-) diff --git a/cli/cmd/definition.go b/cli/cmd/definition.go index b810e04..f68bdb7 100644 --- a/cli/cmd/definition.go +++ b/cli/cmd/definition.go @@ -4,6 +4,7 @@ import ( "encoding/json" "errors" "fmt" + "net/url" "os" "path/filepath" "strconv" @@ -386,6 +387,7 @@ Examples: stackctl definition update-chart 1 5 --chart-path /charts/kvk-core stackctl definition update-chart 1 5 --deploy-order 6 stackctl definition update-chart 1 5 --source-repo-url https://dev.azure.com/org/project/_git/repo + stackctl definition update-chart 1 5 --repository-url oci://acr.example.com/helm stackctl definition update-chart 1 5 --file values.yaml`, Args: cobra.ExactArgs(2), SilenceUsage: true, @@ -410,6 +412,18 @@ Examples: return fmt.Errorf("at least one of --chart-path, --chart-version, --source-repo-url, --repository-url, --deploy-order, or --file must be specified") } + if repositoryURL != "" { + u, err := url.Parse(repositoryURL) + if err != nil || u.Scheme == "" || u.Host == "" { + return fmt.Errorf("--repository-url %q is not a valid URL", repositoryURL) + } + switch u.Scheme { + case "oci", "http", "https": + default: + return fmt.Errorf("--repository-url scheme %q is not supported (use oci, http, or https)", u.Scheme) + } + } + if valuesFile != "" { for _, segment := range strings.Split(filepath.ToSlash(valuesFile), "/") { if segment == ".." { @@ -432,7 +446,6 @@ Examples: // doesn't wipe values the user didn't intend to change. Backend // merges absent-from-JSON keys as "preserve"; we still send the // full record because older backends do full PUT replace. - deployOrderCopy := current.DeployOrder req := types.UpdateChartConfigRequest{ ChartName: current.ChartName, RepositoryURL: current.RepoURL, @@ -440,7 +453,7 @@ Examples: ChartVersion: current.ChartVersion, SourceRepoURL: current.SourceRepoURL, BuildPipelineID: current.BuildPipelineID, - DeployOrder: &deployOrderCopy, + DeployOrder: ¤t.DeployOrder, DefaultValues: current.DefaultValues, } diff --git a/cli/cmd/definition_test.go b/cli/cmd/definition_test.go index cc701d7..9f21b95 100644 --- a/cli/cmd/definition_test.go +++ b/cli/cmd/definition_test.go @@ -1012,13 +1012,17 @@ func TestDefinitionUpdateCmd_NoFlags(t *testing.T) { func sampleChartConfig() types.ChartConfig { return types.ChartConfig{ - Base: types.Base{ID: "1", Version: "1"}, - Name: "api", - RepoURL: "https://charts.example.com", - ChartName: "api-chart", - ChartVersion: "2.0.0", - ReleaseName: "api-release", - DefaultValues: "replicas: 1\nimage: app:latest", + Base: types.Base{ID: "1", Version: "1"}, + Name: "api", + RepoURL: "https://charts.example.com", + ChartName: "api-chart", + ChartPath: "/charts/api", + ChartVersion: "2.0.0", + SourceRepoURL: "https://dev.azure.com/org/project/_git/repo", + BuildPipelineID: "pipeline-42", + DeployOrder: 8, + ReleaseName: "api-release", + DefaultValues: "replicas: 1\nimage: app:latest", } } @@ -1040,6 +1044,15 @@ func TestDefinitionUpdateChartCmd_ChartVersion(t *testing.T) { require.NoError(t, json.NewDecoder(r.Body).Decode(&body)) assert.Equal(t, "3.0.0", body.ChartVersion) assert.Equal(t, chart.ChartName, body.ChartName) + // Fields not touched by the user flag must round-trip from the GET + // response back into the PUT body — that's the wipe-fix this PR exists for. + assert.Equal(t, chart.RepoURL, body.RepositoryURL) + assert.Equal(t, chart.ChartPath, body.ChartPath) + assert.Equal(t, chart.SourceRepoURL, body.SourceRepoURL) + assert.Equal(t, chart.BuildPipelineID, body.BuildPipelineID) + assert.Equal(t, chart.DefaultValues, body.DefaultValues) + require.NotNil(t, body.DeployOrder) + assert.Equal(t, chart.DeployOrder, *body.DeployOrder) chart.ChartVersion = "3.0.0" json.NewEncoder(w).Encode(chart) @@ -1052,6 +1065,8 @@ func TestDefinitionUpdateChartCmd_ChartVersion(t *testing.T) { t.Cleanup(func() { definitionUpdateChartCmd.Flags().Set("chart-version", "") definitionUpdateChartCmd.Flags().Set("chart-path", "") + definitionUpdateChartCmd.Flags().Set("source-repo-url", "") + definitionUpdateChartCmd.Flags().Set("repository-url", "") definitionUpdateChartCmd.Flags().Set("deploy-order", "-1") definitionUpdateChartCmd.Flags().Set("file", "") }) @@ -1089,6 +1104,8 @@ func TestDefinitionUpdateChartCmd_ValuesFromFile(t *testing.T) { t.Cleanup(func() { definitionUpdateChartCmd.Flags().Set("chart-version", "") definitionUpdateChartCmd.Flags().Set("chart-path", "") + definitionUpdateChartCmd.Flags().Set("source-repo-url", "") + definitionUpdateChartCmd.Flags().Set("repository-url", "") definitionUpdateChartCmd.Flags().Set("deploy-order", "-1") definitionUpdateChartCmd.Flags().Set("file", "") }) @@ -1121,6 +1138,8 @@ func TestDefinitionUpdateChartCmd_DeployOrder(t *testing.T) { t.Cleanup(func() { definitionUpdateChartCmd.Flags().Set("chart-version", "") definitionUpdateChartCmd.Flags().Set("chart-path", "") + definitionUpdateChartCmd.Flags().Set("source-repo-url", "") + definitionUpdateChartCmd.Flags().Set("repository-url", "") definitionUpdateChartCmd.Flags().Set("deploy-order", "-1") definitionUpdateChartCmd.Flags().Set("file", "") }) @@ -1129,6 +1148,67 @@ func TestDefinitionUpdateChartCmd_DeployOrder(t *testing.T) { require.NoError(t, err) } +func TestDefinitionUpdateChartCmd_RepositoryURL(t *testing.T) { + chart := sampleChartConfig() + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if r.Method == http.MethodGet { + json.NewEncoder(w).Encode(chart) + return + } + var body types.UpdateChartConfigRequest + require.NoError(t, json.NewDecoder(r.Body).Decode(&body)) + assert.Equal(t, "oci://acr.example.com/helm", body.RepositoryURL) + // Other fields still seeded from GET. + assert.Equal(t, chart.ChartPath, body.ChartPath) + assert.Equal(t, chart.BuildPipelineID, body.BuildPipelineID) + + chart.RepoURL = body.RepositoryURL + json.NewEncoder(w).Encode(chart) + })) + defer server.Close() + + _ = setupStackTestCmd(t, server.URL) + + definitionUpdateChartCmd.Flags().Set("repository-url", "oci://acr.example.com/helm") + t.Cleanup(func() { + definitionUpdateChartCmd.Flags().Set("chart-version", "") + definitionUpdateChartCmd.Flags().Set("chart-path", "") + definitionUpdateChartCmd.Flags().Set("source-repo-url", "") + definitionUpdateChartCmd.Flags().Set("repository-url", "") + definitionUpdateChartCmd.Flags().Set("deploy-order", "-1") + definitionUpdateChartCmd.Flags().Set("file", "") + }) + + err := definitionUpdateChartCmd.RunE(definitionUpdateChartCmd, []string{"5", "1"}) + require.NoError(t, err) +} + +func TestDefinitionUpdateChartCmd_RepositoryURL_Invalid(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t.Fatal("API should not be called when --repository-url is invalid") + })) + defer server.Close() + + _ = setupStackTestCmd(t, server.URL) + + t.Cleanup(func() { + definitionUpdateChartCmd.Flags().Set("repository-url", "") + }) + + cases := []string{ + "not-a-url", + "oci:/acr.example.com/helm", + "ftp://acr.example.com/helm", + } + for _, raw := range cases { + definitionUpdateChartCmd.Flags().Set("repository-url", raw) + err := definitionUpdateChartCmd.RunE(definitionUpdateChartCmd, []string{"5", "1"}) + require.Error(t, err, raw) + assert.Contains(t, err.Error(), "--repository-url") + } +} + func TestDefinitionUpdateChartCmd_NoFlags(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { t.Fatal("API should not be called when no flags provided") @@ -1140,6 +1220,8 @@ func TestDefinitionUpdateChartCmd_NoFlags(t *testing.T) { t.Cleanup(func() { definitionUpdateChartCmd.Flags().Set("chart-version", "") definitionUpdateChartCmd.Flags().Set("chart-path", "") + definitionUpdateChartCmd.Flags().Set("source-repo-url", "") + definitionUpdateChartCmd.Flags().Set("repository-url", "") definitionUpdateChartCmd.Flags().Set("deploy-order", "-1") definitionUpdateChartCmd.Flags().Set("file", "") }) @@ -1169,6 +1251,8 @@ func TestDefinitionUpdateChartCmd_JSONOutput(t *testing.T) { t.Cleanup(func() { definitionUpdateChartCmd.Flags().Set("chart-version", "") definitionUpdateChartCmd.Flags().Set("chart-path", "") + definitionUpdateChartCmd.Flags().Set("source-repo-url", "") + definitionUpdateChartCmd.Flags().Set("repository-url", "") definitionUpdateChartCmd.Flags().Set("deploy-order", "-1") definitionUpdateChartCmd.Flags().Set("file", "") }) @@ -1200,6 +1284,8 @@ func TestDefinitionUpdateChartCmd_QuietOutput(t *testing.T) { t.Cleanup(func() { definitionUpdateChartCmd.Flags().Set("chart-version", "") definitionUpdateChartCmd.Flags().Set("chart-path", "") + definitionUpdateChartCmd.Flags().Set("source-repo-url", "") + definitionUpdateChartCmd.Flags().Set("repository-url", "") definitionUpdateChartCmd.Flags().Set("deploy-order", "-1") definitionUpdateChartCmd.Flags().Set("file", "") }) diff --git a/cli/pkg/types/types.go b/cli/pkg/types/types.go index 2c50d81..77f8d87 100644 --- a/cli/pkg/types/types.go +++ b/cli/pkg/types/types.go @@ -632,8 +632,9 @@ type UpdateDefinitionRequest struct { // // Includes every field the backend's ChartConfig knows about, so a // GET-then-PUT round-trip in update-chart doesn't silently wipe fields -// stackctl flags don't expose. Backend treats absent JSON keys as -// "preserve existing"; explicit "" replaces. +// stackctl flags don't expose. All string fields use omitempty, so an +// empty value is dropped from the JSON body and the backend preserves +// its existing value; there is no way to clear a field via this request. type UpdateChartConfigRequest struct { ChartName string `json:"chart_name,omitempty" yaml:"chart_name,omitempty"` RepositoryURL string `json:"repository_url,omitempty" yaml:"repository_url,omitempty"` From e2ffbc8691a87b82b7449230d22e62f5fb34fa32 Mon Sep 17 00:00:00 2001 From: Olof Mattsson Date: Wed, 24 Jun 2026 07:23:18 +0200 Subject: [PATCH 3/3] docs(definition): trim misleading backend-merge claim in update-chart CodeRabbit flagged that the seed comment described follow-up backend behavior ("merges absent-from-JSON keys as preserve") that isn't a safe assumption to document in this PR. Keep only the rationale that actually applies: send the full record so a full-replace PUT can't wipe fields the user didn't touch. Co-Authored-By: Claude Opus 4.7 (1M context) --- cli/cmd/definition.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/cli/cmd/definition.go b/cli/cmd/definition.go index f68bdb7..d909773 100644 --- a/cli/cmd/definition.go +++ b/cli/cmd/definition.go @@ -442,10 +442,9 @@ Examples: return fmt.Errorf("fetching current chart config: %w", err) } - // Seed every field from the current record so a PUT round-trip - // doesn't wipe values the user didn't intend to change. Backend - // merges absent-from-JSON keys as "preserve"; we still send the - // full record because older backends do full PUT replace. + // Seed every field from the current record before PUT so backends + // that treat the payload as a full replacement do not wipe values + // the user didn't intend to change. req := types.UpdateChartConfigRequest{ ChartName: current.ChartName, RepositoryURL: current.RepoURL,