Skip to content

add support for config set --if-not-set#1873

Merged
lionello merged 2 commits intomainfrom
jordan/config-if-not-set
Jan 27, 2026
Merged

add support for config set --if-not-set#1873
lionello merged 2 commits intomainfrom
jordan/config-if-not-set

Conversation

@jordanstephens
Copy link
Member

@jordanstephens jordanstephens commented Jan 27, 2026

Description

In order to support initial config values for 1-click deploy, this PR adds support for config set --if-not-set. See DefangLabs/defang-github-action#38

Linked Issues

Checklist

  • I have performed a self-review of my code
  • I have added appropriate tests
  • I have updated the Defang CLI docs and/or README to reflect my changes, if necessary

Summary by CodeRabbit

  • New Features
    • Added a --if-not-set option to the config set command: when supplied, the command will skip updating an existing setting and emit a skip message instead of overwriting.
  • Tests
    • Added unit tests covering set, skip, overwrite, invalid-name, and dry-run scenarios.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 27, 2026

📝 Walkthrough

Walkthrough

Adds an --if-not-set flag to the CLI config set command, introduces ConfigSetOptions and ConfigManager, and changes ConfigSet to return a boolean indicating whether a value was set; updates call sites and tests to use the new API and behavior.

Changes

Cohort / File(s) Summary
CLI Flag Definition
src/cmd/cli/command/commands.go
Adds new --if-not-set boolean flag (default false) to the config set subcommand.
CLI Handler
src/cmd/cli/command/config.go
Reads --if-not-set, passes ConfigSetOptions to ConfigSet, and handles three outcomes: error, didSet (updated), or skipped when IfNotSet prevents overwrite (prints skip message).
Core Logic
src/pkg/cli/configSet.go
Adds ConfigSetOptions and ConfigManager interface. Changes ConfigSet signature to accept ConfigManager and options, return (bool, error). Implements IfNotSet check via ListConfig, guarded PutConfig, and revised error returns.
Agent Tool Integration
src/pkg/agent/tools/default_tool_cli.go
Updates internal call to cli.ConfigSet to pass ConfigSetOptions{} and ignore the new boolean return where appropriate.
Tests
src/pkg/cli/configSet_test.go
Replaces concrete provider with mockConfigManager; adds table-driven tests for scenarios: not set (sets), already set (skip), overwrite allowed, and invalid name; asserts didSet flag and error behavior (including dry-run).

Sequence Diagram(s)

sequenceDiagram
  participant CLI as CLI (command/config.go)
  participant ConfigSet as ConfigSet (pkg/cli/configSet.go)
  participant CM as ConfigManager (ListConfig / PutConfig)

  CLI->>ConfigSet: Call ConfigSet(name, value, ConfigSetOptions{IfNotSet})
  alt IfNotSet == true
    ConfigSet->>CM: ListConfig(project, filter=name)
    CM-->>ConfigSet: returns existing configs
    alt config exists
      ConfigSet-->>CLI: return (false, nil)  -- skip, no PutConfig
    else config missing
      ConfigSet->>CM: PutConfig(name, value)
      CM-->>ConfigSet: success
      ConfigSet-->>CLI: return (true, nil)
    end
  else IfNotSet == false
    ConfigSet->>CM: PutConfig(name, value)
    CM-->>ConfigSet: success or error
    ConfigSet-->>CLI: return (true, nil) or (false, err)
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • KevyVo

Poem

🐰 A flag hops in, gentle and neat,
"If-not-set" keeps old configs sweet,
Managers check, and setters wait,
Tests clap paws — no accidental overwrite! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'add support for config set --if-not-set' accurately and concisely summarizes the main change: adding a new CLI flag --if-not-set to the config set command.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.5.0)

level=warning msg="[linters_context] running gomodguard failed: unable to read module file go.mod: current working directory must have a go.mod file: if you are not using go modules it is suggested to disable this linter"
level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"


Comment @coderabbitai help to get the list of available commands and usage tips.

Co-authored-by: Lio李歐 <lionello@users.noreply.github.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/cmd/cli/command/config.go (1)

169-186: Success count does not account for skipped configs.

When --if-not-set is used, configs that already exist are skipped (not errors), but the count len(envMap)-len(errs) still includes them. This results in misleading output like "Successfully set 3 config value(s)" when only 1 was actually set.

Proposed fix: track actual set count
 		var errs []error
+		var setCount int
 		for name, value := range envMap {
 			didSet, err := cli.ConfigSet(cmd.Context(), projectName, session.Provider, name, value, cli.ConfigSetOptions{
 				IfNotSet: ifNotSet,
 			})
 			if err != nil {
 				errs = append(errs, err)
 			} else if ifNotSet && !didSet {
 				term.Info("Config", name, "is already set; skipping due to --if-not-set flag")
 			} else {
+				setCount++
 				term.Info("Updated value for", name)
 			}
 		}

-		term.Infof("Successfully set %d config value(s)", len(envMap)-len(errs))
+		term.Infof("Successfully set %d config value(s)", setCount)

@lionello lionello merged commit d1e3805 into main Jan 27, 2026
14 checks passed
@lionello lionello deleted the jordan/config-if-not-set branch January 27, 2026 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants