acc: Implement config merge #2294
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
denik
force-pushed
the
denik/test-config-merge
branch
from
94d2cf0 to
f65b5bd
Compare
denik
requested review from
pietern,
andrewnester and
shreyas-goenka
as code owners
pietern
reviewed
Feb 7, 2025
| dario.cat/mergo | ||
| Copyright (c) 2013 Dario Castañé. All rights reserved. | ||
| Copyright (c) 2012 The Go Authors. All rights reserved. | ||
| https://github.com/darccio/mergo/blob/master/LICENSE |
There was a problem hiding this comment.
Historical tidbit: we used to depend on this bundle configuration merging before the move to libs/dyn.
There was a problem hiding this comment.
Thanks. It looks solid. From the past experience, are there any issues or quirks to be aware of?
| if dir == "" || dir == "." { | ||
| break | ||
| } | ||
|
|
||
| if os.IsNotExist(err) { | ||
| dir = filepath.Dir(dir) | ||
| dir = filepath.Dir(dir) |
There was a problem hiding this comment.
Is the path guaranteed to never be absolute?
If it is, this loop will be infinite.
There was a problem hiding this comment.
Yes, in this context is always relative. If it was absolute, the logic would not work anyway as we don't want to search for configs outside of acceptance/.
Added a comment about this requirement.
acceptance/config_test.go
Outdated
| // FindConfig finds the closest config file. | ||
| func FindConfig(t *testing.T, dir string) (string, bool) { | ||
| shared := false | ||
| // FindConfigs finds the closest config file. |
| continue | ||
| } | ||
|
|
||
| t.Fatalf("Error while reading %s: %s", path, err) | ||
| } | ||
|
|
||
| t.Fatal("Config not found: " + configFilename) | ||
| return "", shared | ||
| slices.Reverse(configs) |
There was a problem hiding this comment.
This can include a comment that the output will be ordered from outer to inner configuration.
There was a problem hiding this comment.
Included in function description.
pietern
approved these changes
Feb 7, 2025
All configs using all test.toml found from acceptance/test.toml to acceptance/<specific test>/test.toml Maps are merged, slices are appended, other values are overriden. I had to disable caching, because it is tricky when merging is involved - deep copy is needed. There is performance impact but currently it is tiny: ~/work/cli/acceptance % hyperfine -w 2 'go test -count=1' # this change: Benchmark 1: go test -count=1 Time (mean ± σ): 5.074 s ± 0.105 s [User: 6.189 s, System: 9.551 s] Range (min … max): 4.926 s … 5.263 s 10 runs ~/work/cli/acceptance % git stash # base ~/work/cli/acceptance % hyperfine -w 2 'go test -count=1' Benchmark 1: go test -count=1 Time (mean ± σ): 5.009 s ± 0.095 s [User: 6.176 s, System: 10.617 s] Range (min … max): 4.803 s … 5.123 s 10 runs
denik
force-pushed
the
denik/test-config-merge
branch
from
f3c155a to
8e16c80
Compare
denik
force-pushed
the
denik/test-config-merge
branch
from
8e16c80 to
d14d075
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue bot
pushed a commit
that referenced
this pull request
Feb 7, 2025
Makes use of #2294
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
Instead of using leaf-most config, all configs from root at acceptance/test.toml to all intermediate ones to leaf config are merged into one. Maps are merged, slices are appended, other values are overridden.
I had to disable caching, because it is tricky when merging is involved - deep copy is needed. There is performance
impact but currently it is tiny, about 1%.
Also, remove empty root config.
Tests
Manually checked that inheritance of LocalOnly setting worked for these tests:
Before - integration tests showed:
After: