refactor: implement config model using viper, fixes #5763 (#8181) [skip ci]

[agviu]

The Issue

• Fixes Rework DDEV configuration model using spf13/viper (or similar) #5763

Use viper completely for config.

How This PR Solves The Issue

Introduces a new pkg/settings package that uses Viper as a unified YAML configuration loader. This replaces the manual os.ReadFile + yaml.Unmarshal loops in config.go and global_config.go with a structured, testable abstraction.

• This PR strictly limits the scope to existing behavior in .ddev/config.yaml and ~/.ddev/global_config.yaml loading.
• Environment variables (AutomaticEnv, BindEnv) are intentionally excluded and deferred to a future PR to prevent regressions and maintain stability.
• project_list.yaml loading remains untouched. An earlier attempt in this branch to parse the project list via Viper was reverted because Viper's default behavior of lowercasing all map keys corrupted mixed-case project names.
• Because the scope has been restricted specifically to YAML file parsing with no environment variable bindings, the internal API is unified: all files are loaded using a standard Viper provider (NewConfigProvider) without the need for distinct "clean" profiles.

What changed

New: pkg/settings/

• config.go — Defines the isolated ConfigProvider and ProviderFactory interfaces, plus high-level package functions like LoadGlobalConfig and LoadProjectConfig.
• viper.go — Implements the interfaces using Viper.
  • Float Preservation Hook: Includes a floatToStringHook that preserves decimal precision when YAML values like 8.0 are unmarshaled into string fields (Viper/mapstructure would otherwise truncate this to "8").
  • Custom Config Merging (RecursiveMerge): Historically, DDEV relied on yaml.Unmarshal's native behavior of incrementally deep-merging arrays and maps when called multiple times on the same DdevApp struct. When migrating to Viper, we lost this implicit struct-merging behavior, as Viper tends to replace complex collections rather than deep-merging them. To maintain 100% backward compatibility with how users expect config.*.yaml overrides to behave, we now parse the main config and overrides into independent map[string]any representations, use a custom RecursiveMerge function to explicitly and safely deep-merge the maps and append slices, and finally decode the merged result into the target struct.
• config_test.go / viper_test.go — Comprehensive test suite covering config loading, factory isolation, float precision, and struct unmarshaling. TestLoadProjectConfig explicitly tests that RecursiveMerge correctly deep-merges map configurations (like web_environment) and properly appends slices (like hooks) during override parsing.

Modified: pkg/ddevapp/config.go

• ReadConfig and LoadConfigYamlFile now delegate entirely to settings.LoadProjectConfig(app.ConfigPath, overrides, app) instead of executing manual YAML unmarshaling loops. Overrides are processed securely via RecursiveMerge.
• Pre-load validation for hooks is now explicitly done before passing files to the settings loader to ensure safe parsing.
• Removed mergeAdditionalConfigIntoApp as its logic is now safely encapsulated within the settings package.

Modified: pkg/globalconfig/global_config.go

• ReadGlobalConfig now delegates entirely to settings.LoadGlobalConfig(globalConfigFile, &DdevGlobalConfig). This function is intentionally kept separate from LoadProjectConfig because it parses a single file with no dynamic overrides, and therefore avoids the complexity of the RecursiveMerge logic.

Manual Testing Instructions

Use ddev normally, everything should work as always. Verify that project overrides (like config.override.yaml) correctly append slice data (e.g., hooks) and merge map data (e.g., web_environment) without replacing the original collections.

Automated Testing Overview

• All automated tests related to configuration overriding, array appending, and map merging pass perfectly locally.
• During CI testing, I ran all the affected tests and noticed some seemingly unrelated issues (like timeouts). I compared the tests that run in the master branch vs this branch and the output was the same. In particular, the tests TestConfigValidate, TestPHPConfig, and TestTimezoneConfig fail identically in master as they do here.

Release/Deployment Notes

Nothing special.

[github-actions]

Download the artifacts for this pull request:

• ddev-windows-arm64-installer.zip
• ddev-windows-arm64.zip
• ddev-windows-amd64-installer.zip
• ddev-windows-amd64.zip
• ddev-linux-amd64.zip
• ddev-linux-arm64.zip
• ddev-macos-arm64.zip
• ddev-macos-amd64.zip
• all-ddev-executables.zip

See Testing a PR.

[rfay]

Please take a look through the places where you changed logic and spend some time improving the OP to explain the underlying reason. A casual look through shows that config merging, which has a long and difficult history (and may not have enough testing) had significant logic changes. Look through at things like that and look at the reasoning. In places where the logic has changed, this PR really needs to explain why.

BTW, I really appreciate limiting the scope to config with existing behavior. That was the right thing to do.

[agviu]

Hi @rfay ,
I have updated the OP. Hopefully it's more clear now. I added a specific section about the config merging (overrides).

[rfay]

Rebased

[rfay]

I haven't fully studied this, but gave it to Claude for a review.

The general approach is sound, but there are behavioral regressions where test data was changed to accept the new (broken) behavior rather than preserving the old behavior.

Is it legit that the testdata was changed?

Do we need more testing for override_config?

---

Critical Issues

1. override_config: true is silently broken

This is the most serious issue. The old mergeAdditionalConfigIntoApp in config_merge.go explicitly handled OverrideConfig:

if newConfig.OverrideConfig {
    err = app.LoadConfigYamlFile(configPath) // full replacement
} else {
    err = mergo.Merge(app, newConfig, mergo.WithAppendSlice, mergo.WithOverride)
}

The new code in RecursiveMerge always deep-merges/appends, and never checks OverrideConfig. So a config.override.yaml with override_config: true is silently ignored — it used to allow resetting arrays to [] or overriding boolean values that merging could not touch.

The test expected data was changed to accept this regression. In fulloverridetest/expected/.ddev/config.yaml, additional_hostnames used to be [overrideone, overridetwo, z-one] (override replaced base) but is now [baseone, basetwo, basethree, overrideone, overridetwo, z-one] (all merged together). The feature is documented in templates.go but no longer works.

2. Duplicate env vars no longer deduplicated

The old mergeAdditionalConfigIntoApp called EnvToUniqueEnv and SliceToUniqueSlice after merging, ensuring last-write-wins for duplicate web_environment entries like ORIG_TO_BE_OVERWRITTEN=orig + ORIG_TO_BE_OVERWRITTEN=last.

The new code has no such deduplication. The test expected data for envtest was changed to add ORIG_TO_BE_OVERWRITTEN=orig as a duplicate (where previously =last would win). The comment # This one should win still exists in the source config but the expected output now shows both values — and containers will see the first-defined value, not the intended override.

3. LoadConfigYamlFile semantics changed without clear justification

The original contract was "load one specific file." The new version auto-globs for config.*.yaml in the same directory. This is a hidden behavior change for callers.

To accommodate this, TestPkgConfigDatabaseDBVersion was changed from:

err = app.LoadConfigYamlFile(configFile)  // test that this specific file's DB version is correct

to:

_, err = app.ReadConfig(false)  // different function, different semantics

This masks that LoadConfigYamlFile changed behavior.

---

Moderate Issues

4. Global package-level state in pkg/settings

The package has top-level config and factory vars initialized in init(). This is a global singleton, which can cause test pollution. Tests in config_test.go correctly restore state via defer, but anyone using settings.Set(...) or settings.Init() globally could affect other test runs.

5. Tests use assert instead of require

All new tests in pkg/settings/config_test.go and pkg/settings/viper_test.go use assert, which is inconsistent with DDEV project standards. Test failures won't stop execution on the first assertion failure.

6. Double file reading in ReadConfig

The new ReadConfig reads each config file twice: once via os.ReadFile for validateHookYAML, and then again through Viper. Minor inefficiency but worth noting.

---

What Works Well

• The floatToStringHook correctly solves the 8.0 → "8" truncation problem
• The RecursiveMerge logic is clean and the intent is sound
• Viper not calling AutomaticEnv is the right choice and is tested
• TestViperUnmarshalDoesNotPickUpEnv is a good guard against future regressions
• The float preservation tests are thorough

---

Bottom Line

The two test data changes in envtest/expected and fulloverridetest/expected are the key red flags — they indicate the tests were adjusted to match broken behavior rather than fixing the implementation. The override_config: true feature and env var deduplication need to be preserved before this can be merged. The rest of the approach is reasonable.

[agviu]

Hi @rfay , I worked on the concerns from your comment. I pushed the changes. This is a summary:

• Restored override_config: true: Config files now correctly replace slices instead of appending when this flag is enabled.
• Improved Deduplication: Re-added "last-write-wins" logic for web_environment and uniqueified array slices after merging.
• Restored LoadConfigYamlFile Semantics: Reverted the function to its original contract of loading exactly one specific file.
• Standardized Test Assertions: Updated pkg/settings tests to use require instead of assert, aligning with DDEV project standards.
• Optimized I/O Performance: Refactored ReadConfig to avoid double-reading files. Contents are now read once, validated, and passed directly to the configuration loader.

About the concern 4th, I have to check in the future.

[agviu]

@rfay I added the patch about the global state, and now it's stateless. Please check again.

[agviu]

The errors are unrelated to my branch, @rfay

[stasadev]

Rebased and resolved conflicts.

I created two commits so that we could easily split the vendor files in another PR.

[agviu]

Thanks @stasadev . Please notice, that the error in the test is unrelated to my branch.

[rfay]

There's no reason to change this for this PR. Please make sure in a hefty PR that you change only things relevant to the PR.

There's an old issue about not requiring mysql on host for testing, it would be OK to address with a PR for that, but not here.

[rfay]

Thanks for the work on this. I still have only casually looked.

@stasadev is working elsewhere on a massive change that will affect all vendor stuff. He brought that in in

• #8198

If you haven't already rebased to address that, it's a good time.

He also has developed a technique of separating vendor changes into a single commit to make everything more reviewable, it's a good idea.

[rfay]

For this important PR, it's critical to show that all the existing tests work without change. If you actually have a reason for changing something, it needs significant comment.

[agviu]

Hi, yes you are right. It was temporarily added to test a slice merging issue during the configuration refactor. Once the underlying merging logic was properly fixed and override_config was properly discarded, this line became an unnecessary leftover and has been removed.

[stasadev]

Thanks for the work on this. I still have only casually looked.

@stasadev is working elsewhere on a massive change that will affect all vendor stuff. He brought that in in

• build: update go deps, vendor docker-compose SDK, for #8234 #8198

If you haven't already rebased to address that, it's a good time.

He also has developed a technique of separating vendor changes into a single commit to make everything more reviewable, it's a good idea.

It's already rebased.

I'm going to convert #8237 into bumping the vendor dir for viper. And #8237 can be merged in right away, as my changes to the vendor don't increase the actual size of the binary if the import isn't actually used.

[stasadev]

Rebased.

[agviu]

Thanks @stasadev , I also reverted the commit that was changing the test, as requested by @rfay .
Still, that test was constantly failing in my environment. If there's another issue/PR where this is being addressed, I could add my commit there.

[agviu]

I checked the last error, it's again non related to this branch.

[rfay]

@agviu the podman rootless test fails quite a lot. I added you as collaborator on this repo. If you accept the invite, does that give you privs to restart the test? (I see you already have privileges for buildkite tests)

[agviu]

Thanks, @rfay. I accepted the invitation, but I don't see any new option or icon about restarting the test. Could you let me know where is usually this option in the GIthub's interface?

[rfay]

@agviu I updated your role to triage, don't know if it will help. If it doesn't give you perms to restart the test I'll do it.

[agviu]

@rfay , I still cannot see any option for restarting the test...

[rfay]

Too bad. I restarted it.

[stasadev]

Restarting jobs requires write access https://github.com/orgs/community/discussions/189061

[rfay]

Thanks for all the careful work and followup on this.

• It seems that this adds a 3rd yaml dependency, which isn't great, but at least they've moved to the supported one.

• I would probably prefer if pkg/settings/config.go had the name settings_config.go, (or something different) since in the IDE it's already too easy to open the wrong one. But can live with it as is, especially if you think that's important

• I liked Mergo :)

• viper.go's RecursiveMerge seems like the most risk but tests seem to validate it.

• Since tests have not been changed this should be pretty safe.

• What about the concerns that viper is outdated and losing favor? I think that conversation probably should have been finished before this PR got all this energy, but I think it's going to be OK. Is viper a friend of yours that you have a lot of experience with @agviu ?

• The problem with map management in viper automatically using lowercase seems like a bit of risk, and has a good workaround. Does it seem like a risk? Mixed case could appear other places than project_list.yaml couldn't it?

• FUTURE TODO: We really would like to eliminate the copied code for elements that are in both global and project, and also allow more global config (hooks would be a candidate for example)

I approve. It may be some time before Stas has time to fully look still. Hi from the train near Glenwood Springs, Colorado, where I've been traveling back home from Chicago for about 24 hours.

[agviu]

HI @rfay , thanks for your review and comments.

• I changed the name as you suggested, makes sense.
• I have used viper in a project in my company. I think it works as expected. I don't use its full power, as it brings many features. We're not doing it in DDEV either. Being said that, the implementation is modular through the ConfigProvider interface, with the goal of easily changing it in the future if needed.
• About the lowercase keys, yes, it could be potentially used somewhere else. We'd have to be careful with this in the future. In the case of project_list.yaml, the key's were ultimately generated by the end-user as they can freely choose the project's name. Other configurations could be more restrictive in this regard, so it doen't cause problems.
• Future TODO's: yes, fully agree. I think this refactor is a first step into removing that duplicated code.

Finally, 24 hours by train seems like a very long trip. I hope you arrived safe back at home!

[rfay]

Thanks! Sorry for the filename rename request. Seems kind of silly, but it's appreciated. I'm glad to know that Viper is familiar ground for you. That experience brings more safety to this PR.

[stasadev]

Looks good to me, thank you!

I have a few small nitpicks about the functions.

[stasadev]

mainPath is passed to LoadProjectConfigFromContents but never used - it operates entirely on mainContent.
Was this intentional (reserved for something in the future), or can we drop it from the signature?

[stasadev]

It looks like the EnvToUniqueEnv function is the only one in this file.

I suggest moving it next to util.SliceToUniqueSlice (pkg/util/utils.go), along with its corresponding test TestEnvToUniqueEnv.

[agviu]

Hi @stasadev ,
thanks for your comments. I have updated the code based on your suggestions, because they make sense. In particular, the mainPath unused variable seems to be a leftover from a previous refactoring. Good catch!

[stasadev]

Looks good to me.

[rfay]

Wow, what an amazing step.