Apply built-in defaults when reusing an existing cluster manifest by kovayur · Pull Request #308 · stackrox/roxie

kovayur · 2026-10-13T14:40:19Z

Problem

roxie deploy central fails against a cluster that already has a stored manifest secret:

Error: roxie.imageRegistry must include a registry host and org path (e.g. quay.io/rhacs-eng), got:

Root cause

assembleConfigForCommand used an existing cluster manifest directly as the base config (config = *configBase), bypassing deployer.DefaultConfig(). Manifests written by an older roxie (before the imageRegistry field from ROX-35434) have an empty roxie.imageRegistry. Since UsesCustomRegistry() returns ImageRegistry != DefaultRegistry, an empty string is treated as a custom registry, so validateImageRegistry("") runs and fails with the empty got:.

Fix

Always start from deployer.DefaultConfig() and merge the cluster manifest (then user config, then args) on top of it. This preserves built-in defaults when the manifest omits them, while non-empty manifest values still take precedence — matching the existing configBase == nil layering.

Testing

Added TestAssembleConfigForCommand (fails before the fix, passes after).
go build ./..., go test ./..., go vet, and gofmt all clean.

🤖 Generated with Claude Code

Summary by CodeRabbit

Bug Fixes
- Command configurations now retain default values when a base configuration is provided, while preserving custom base values and overrides.

When deploying onto a cluster that already had a stored manifest secret, assembleConfigForCommand used that manifest directly as the base config, bypassing deployer.DefaultConfig(). Manifests written by an older roxie (predating the imageRegistry field) left roxie.imageRegistry empty, which UsesCustomRegistry() treats as a custom registry, causing deploy to fail validation with "roxie.imageRegistry must include a registry host and org path (e.g. quay.io/rhacs-eng), got:". Always start from the built-in defaults and merge the manifest (and user config, then args) on top, so defaults are preserved when the manifest omits them. Non-empty manifest values still take precedence. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

coderabbitai · 2026-10-13T14:40:40Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: stackrox/roxie/.coderabbit.yml
Review profile: CHILL
Plan: Advanced
Run ID: 57bcf8e5-27db-44db-975d-09e37096c7c1

📥 Commits

Reviewing files that changed from the base of the PR and between 7931024 and bafb0e6.

📒 Files selected for processing (2)

cmd/config.go
cmd/deploy_test.go

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Walkthrough

Command config assembly now starts with built-in defaults and overlays any non-nil base configuration. Tests cover default registry values, preservation of base values, and custom registry precedence.

Changes: Command Configuration

Layer / File(s)	Summary
Default configuration overlay `cmd/config.go`, `cmd/deploy_test.go`	`assembleConfigForCommand` merges a non-nil base configuration over built-in defaults and wraps merge errors. Tests check default registry values and confirm that base values, including a custom registry, take precedence.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: mclasmeier

Merge Risk: ⚪ Minimal · up to bafb0

Existing manifests that omit the image registry should receive the built-in default. No confirmed issue currently blocks merging.

Pre-merge checks · 4 · 1

Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks (4 passed)

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: applying built-in defaults when reusing an existing cluster manifest.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

Commit to this branch
Create a new PR

🧪 Generate unit tests (beta)

Commit to this branch
Create a new PR

_{Comment @coderabbitai help to get the list of available commands.}

AlexVulaj · 2026-10-15T12:37:56Z

Thanks for this PR! I've given my approval but would recommend also waiting for at least one of @porridge or @mclasmeier to take a look as well.

vladbologa

Thanks for finding and fixing this!

vladbologa · 2026-10-16T11:53:16Z

+	if configBase != nil {
+		// Merge the provided base (e.g. an existing cluster manifest) on top of
+		// the defaults; non-empty values in the base take precedence.
+		if err := mergo.Merge(&config, configBase, mergo.WithOverride, mergo.WithoutDereference); err != nil {


This seems ok for now, but could create problems in the future if we add e.g. an int field.
If the default value of that field is 1, and the user tries to set it to 0, it won't be posible.

An alternative would be to keep the direct copy and just add the missing field:

if config.Roxie.ImageRegistry == "" { config.Roxie.ImageRegistry = constants.DefaultRegistry }

But then every new field with a non-zero default needs such a fix. Not sure which one's better. Your approach would always work fine if all config fields were pointers.

would always work fine if all config fields were pointers

@kovayur can you please file a Jira ticket and put it in TODO here?

porridge · 2026-10-19T10:13:37Z

+	// Isolate the user config lookup so it cannot pick up a real file on disk.
+	tmpDir := t.TempDir()
+	t.Setenv("XDG_CONFIG_HOME", tmpDir)
+	t.Setenv("HOME", tmpDir) // For non-Unix systems.


Extract a helper function and reuse it below? (See setupMockAuthEnvironment in internal/dockerauth/dockerauth_test.go).

porridge · 2026-10-19T10:15:57Z

+	if configBase != nil {
+		// Merge the provided base (e.g. an existing cluster manifest) on top of
+		// the defaults; non-empty values in the base take precedence.
+		if err := mergo.Merge(&config, configBase, mergo.WithOverride, mergo.WithoutDereference); err != nil {


would always work fine if all config fields were pointers

@kovayur can you please file a Jira ticket and put it in TODO here?

AlexVulaj approved these changes Oct 15, 2026

View reviewed changes

vladbologa reviewed Oct 16, 2026

View reviewed changes

porridge approved these changes Oct 19, 2026

View reviewed changes

Navigation Menu

Repository navigation

Apply built-in defaults when reusing an existing cluster manifest - #308

Apply built-in defaults when reusing an existing cluster manifest#308
kovayur wants to merge 1 commit into
mainfrom
fix-imageregistry-default-from-manifest

kovayur commented Oct 13, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Oct 13, 2026 •

edited

Loading

Uh oh!

AlexVulaj commented Oct 15, 2026

Uh oh!

vladbologa left a comment

Uh oh!

vladbologa Oct 16, 2026 •

edited

Loading

Uh oh!

porridge Oct 19, 2026

Uh oh!

porridge Oct 19, 2026

Uh oh!

porridge Oct 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

kovayur commented Oct 13, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Root cause

Fix

Testing

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Oct 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexVulaj commented Oct 15, 2026

Uh oh!

vladbologa left a comment

Choose a reason for hiding this comment

Uh oh!

vladbologa Oct 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

porridge Oct 19, 2026

Choose a reason for hiding this comment

Uh oh!

porridge Oct 19, 2026

Choose a reason for hiding this comment

Uh oh!

porridge Oct 19, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kovayur commented Oct 13, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Oct 13, 2026 •

edited

Loading

vladbologa Oct 16, 2026 •

edited

Loading