Apply built-in defaults when reusing an existing cluster manifest

[kovayur]

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.

[coderabbitai]

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]

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]

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.