node: Read config files from dir even if config file not set via --config #245

Merged
fyrchik merged 3 commits from acid-ant/frostfs-node:bugfix/238-fix-read-cfg-from-dir into master 2023-04-14 10:19:11 +00:00
Collaborator

Close #238

Signed-off-by: Anton Nikiforov an.nikiforov@yadro.com

Close #238 Signed-off-by: Anton Nikiforov <an.nikiforov@yadro.com>
acid-ant requested review from storage-core-committers 2023-04-13 09:15:39 +00:00
acid-ant requested review from storage-core-developers 2023-04-13 09:15:40 +00:00
fyrchik approved these changes 2023-04-13 10:54:46 +00:00
@ -188,3 +188,3 @@
FROSTFS_TRACING_ENDPOINT="localhost"
FROSTFS_TRACING_EXPORTER="otlp_grpc"
FROSTFS_TRACING_EXPORTER="otlp_grpc"

What has changed here?

What has changed here?
Poster
Collaborator

New line added.

New line added.
fyrchik marked this conversation as resolved
@ -37,2 +33,2 @@
var cfgFile *os.File
cfgFile, err = os.Open(fileName)
func mergeConfig(v *viper.Viper, fileName string) error {
cv := viper.New()

Have you thought about "just setting empty config map" in the beginning and performing Merge everywhere else?

Have you thought about "just setting empty config map" in the beginning and performing `Merge` everywhere else?
Poster
Collaborator

The main problem with merge with empty map that viper unable to parse incoming data because we do not have an API to set config type for this data. Only if we set it directly at the beginning viper able to parse data which we provide with Merge. But this will block us from merging data in multiple formats.

The main problem with merge with empty map that viper unable to parse incoming data because we do not have an API to set config type for this data. Only if we set it directly at the beginning viper able to parse data which we provide with Merge. But this will block us from merging data in multiple formats.
Poster
Collaborator

They still have an open issue for merge https://github.com/spf13/viper/issues/181

They still have an open issue for merge https://github.com/spf13/viper/issues/181
fyrchik marked this conversation as resolved
ale64bit requested changes 2023-04-13 11:51:06 +00:00
@ -0,0 +10,4 @@
)
func TestConfigDir(t *testing.T) {
dir, err := os.MkdirTemp("", "")
Collaborator
[t.TempDir()](https://pkg.go.dev/testing#T.TempDir)
Poster
Collaborator

Perfect, thanks!

Perfect, thanks!
ale64bit marked this conversation as resolved
@ -0,0 +16,4 @@
cfgFileName0 := path.Join(dir, "cfg_00.json")
cfgFileName1 := path.Join(dir, "cfg_01.yml")
cfgFile, err := os.Create(cfgFileName0)
Collaborator

Consider simplifying to

require.NoError(t, os.WriteFile(cfgFileName0, `{ "storage": { "shard_pool_size": 15 } }` , 0777))

Same below.

Consider simplifying to ``` require.NoError(t, os.WriteFile(cfgFileName0, `{ "storage": { "shard_pool_size": 15 } }` , 0777)) ``` Same below.
Poster
Collaborator

Fixed.

Fixed.
ale64bit marked this conversation as resolved
@ -0,0 +41,4 @@
os.Clearenv() // ENVs have priority over config files, so we do this in tests
return New(p,
Collaborator

inline the value if it's not actually needed:

return New(Prm{}, WithConfigDir(dir))
inline the value if it's not actually needed: ``` return New(Prm{}, WithConfigDir(dir)) ```
Poster
Collaborator

Removed this method at all, looks redundant. Please review.

Removed this method at all, looks redundant. Please review.
ale64bit marked this conversation as resolved
ale64bit reviewed 2023-04-13 12:03:27 +00:00
@ -0,0 +39,4 @@
func fromDir(dir string) *Config {
var p Prm
os.Clearenv() // ENVs have priority over config files, so we do this in tests
Collaborator

it probably would be better to use t.Setenv() here, to avoid nuking the env for every test and/or conflict with parallel ones.

it probably would be better to use [t.Setenv()](https://pkg.go.dev/testing#T.Setenv) here, to avoid nuking the env for every test and/or conflict with parallel ones.
Poster
Collaborator

Removed this method at all, looks redundant.

Removed this method at all, looks redundant.
ale64bit marked this conversation as resolved
acid-ant force-pushed bugfix/238-fix-read-cfg-from-dir from a5fd41bda0 to e694540f2a 2023-04-13 13:00:14 +00:00 Compare
acid-ant requested review from ale64bit 2023-04-13 13:01:39 +00:00
ale64bit approved these changes 2023-04-13 13:10:03 +00:00
@ -0,0 +15,4 @@
cfgFileName0 := path.Join(dir, "cfg_00.json")
cfgFileName1 := path.Join(dir, "cfg_01.yml")
require.NoError(t, os.WriteFile(cfgFileName0, []byte("{\n \"storage\": {\n \"shard_pool_size\": 15\n }\n}"), 0777))
Collaborator

if you use backticks, you don't need to quote the special characters

i.e.
[]byte{`{ "foo" : "bar" }`}

if you use backticks, you don't need to quote the special characters i.e. ```[]byte{`{ "foo" : "bar" }`}```
ale64bit marked this conversation as resolved
acid-ant force-pushed bugfix/238-fix-read-cfg-from-dir from e694540f2a to 76f5b1f18a 2023-04-13 13:15:22 +00:00 Compare
fyrchik requested review from carpawell 2023-04-13 13:18:06 +00:00
acid-ant force-pushed bugfix/238-fix-read-cfg-from-dir from 76f5b1f18a to 187eddf69f 2023-04-13 14:15:56 +00:00 Compare
acid-ant force-pushed bugfix/238-fix-read-cfg-from-dir from 187eddf69f to 2e27f567c5 2023-04-13 14:20:17 +00:00 Compare
acid-ant force-pushed bugfix/238-fix-read-cfg-from-dir from 2e27f567c5 to 0ed3eebe1c 2023-04-13 14:21:31 +00:00 Compare
Poster
Collaborator

Fixed CHANGELOG - moved changes in section related to v0.37.0.

Fixed CHANGELOG - moved changes in section related to v0.37.0.
acid-ant force-pushed bugfix/238-fix-read-cfg-from-dir from 0ed3eebe1c to 4c04395ba9 2023-04-14 06:13:19 +00:00 Compare
fyrchik reviewed 2023-04-14 06:43:25 +00:00
@ -20,4 +21,2 @@
continue
}
ext := path.Ext(entry.Name())
if ext != ".yaml" && ext != ".yml" {

I think we should still have .yaml/.json check -- we don't want to read arbitrary garbage (may be we do, but that is a separate feature).

I think we should still have `.yaml`/`.json` check -- we don't want to read arbitrary garbage (may be we do, but that is a separate feature).
Poster
Collaborator

Reverted, added support for json.

Reverted, added support for json.
fyrchik approved these changes 2023-04-14 06:43:28 +00:00
acid-ant force-pushed bugfix/238-fix-read-cfg-from-dir from 4c04395ba9 to 696cae396c 2023-04-14 09:05:55 +00:00 Compare
acid-ant force-pushed bugfix/238-fix-read-cfg-from-dir from 696cae396c to 13dd4ec563 2023-04-14 09:07:53 +00:00 Compare
fyrchik approved these changes 2023-04-14 10:19:04 +00:00
fyrchik merged commit 995db117d0 into master 2023-04-14 10:19:11 +00:00
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-node#245
There is no content yet.