remove pointer from storage config definition

[joao-zanutto]

No description provided.

[joao-zanutto]

@wwoytenko more context on the test I did:

I added a debug fmt.Println() to the initConfig() function to validate that the env was being unmarshalled and it's value was being loaded:

cmd/greenmask/cmd/root.go
...
func initConfig() {
	if cfgFile != "" {
		viper.SetConfigFile(cfgFile)
		if err := viper.ReadInConfig(); err != nil {
			log.Fatal().Err(err).Msg("error reading from config file")
		}
	}

	viper.SetEnvKeyReplacer(strings.NewReplacer(".", "_"))
	viper.AutomaticEnv()

	decoderCfg := func(cfg *mapstructure.DecoderConfig) {
		cfg.DecodeHook = mapstructure.ComposeDecodeHookFunc(
			configUtils.ParamsToByteSliceHookFunc(),
			mapstructure.StringToTimeDurationHookFunc(),
			mapstructure.StringToSliceHookFunc(","),
		)
	}

	if err := viper.Unmarshal(Config, decoderCfg); err != nil {
		log.Fatal().Err(err).Msg("")
	}
	fmt.Println(viper.Get("storage.s3.region"))
}

Then I ran greenmask:

joao-personal@joao:~/greenmask$ STORAGE_TYPE=s3 STORAGE_S3_BUCKET=ryft-public-sample-data STORAGE_S3_REGION="us-east-2" LOG_LEVEL=debug ./greenm
ask list-dumps
us-east-2
2024-04-04T19:41:46-07:00 DBG internal/storages/s3/s3.go:165 > s3 storage bucket bucket= pid=89261 region=
2024-04-04T19:41:46-07:00 DBG internal/storages/s3/logger.go:33 > s3 storage logging 0="DEBUG: Validate Request s3/ListObjectsV2 failed, not retrying, error MissingRegion: could not find region configuration" pid=89261
2024-04-04T19:41:46-07:00 DBG internal/storages/s3/logger.go:33 > s3 storage logging 0="DEBUG: Build Request s3/ListObjectsV2 failed, not retrying, error MissingRegion: could not find region configuration" pid=89261
2024-04-04T19:41:46-07:00 DBG internal/storages/s3/logger.go:33 > s3 storage logging 0="DEBUG: Sign Request s3/ListObjectsV2 failed, not retrying, error MissingRegion: could not find region configuration" pid=89261
2024-04-04T19:41:46-07:00 FTL cmd/greenmask/cmd/list_dumps/list_dumps.go:46 > error="error listing s3 objects v2: MissingRegion: could not find region configuration" pid=89261```

I don't have valid credentials on this machine, however I did a similar test using the `docker-compose.yaml` file with the embedded minio container and got the same result

[wwoytenko]

It looks like it is impossible to use a nonpointer config. Because the new config structure is created in each stack level because it is passing by value. The only possible solution is to revert the root pointer *Config, otherwise it will now work. If it is not working properly with dynamic env vars unmarshalling then we should forget about it and use the dynamic function as was presented in #51 here

[wwoytenko]

@joao-zanutto I am currently assembling the release description and it is hard to find the changes you've made in your PRs. Could you enrich all open and the next PRs with the description?

[joao-zanutto]

It looks like it is impossible to use a nonpointer config. Because the new config structure is created in each stack level because it is passing by value. The only possible solution is to revert the root pointer *Config, otherwise it will now work. If it is not working properly with dynamic env vars unmarshalling then we should forget about it and use the dynamic function as was presented in #51 here

I'd argue to keep #54 as it would fix all the other env variables. S3 storage variables would be the only variables that we need to bound manually (as done in #51) if we keep the build flag

[wwoytenko]

I wonder why Storage config can't be mapped with variables? We can get rid of the pointer in the S3 and Directory storage configs structure. The only thing that cannot work is if we use non pointer root Config structure.

P.S.

If you think changes in this MR is not required anymore you can close it. We can continue conversation in #54

[joao-zanutto]

I wonder why Storage config can't be mapped with variables? We can get rid of the pointer in the S3 and Directory storage configs structure. The only thing that cannot work is if we use non pointer root Config structure.

P.S.

If you think changes in this MR is not required anymore you can close it. We can continue conversation in #54

Actually, it's only s3 config that is not working, setting STORAGE_DIRECTORY_PATH works correctly. If you take a look at the test I did in my first comment, I was able to set a STORAGE_S3_REGION and then print it's value with fmt.Println(viper.Get("storage.s3.region")) and it was correct, however, in the debug log just below, I see that the region is blank

I'll keep debugging and will let you know if anything changes

[joao-zanutto]

@wwoytenko I found the culprit: having omitempty on the mapstructure attribute definition in the Config struct was causing the env vars to be ignored if the field was not declared in the config file. I've added a fix to #54

Closing this pull request as these changes are irrelevant for the fix