Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use default toml if config file without ext #330

Merged
merged 2 commits into from
May 24, 2023
Merged

use default toml if config file without ext #330

merged 2 commits into from
May 24, 2023

Conversation

jarily
Copy link
Contributor

@jarily jarily commented May 23, 2023

No description provided.

@kl7sn kl7sn requested a review from sevennt May 23, 2023 07:48
@sevennt
Copy link
Contributor

sevennt commented May 23, 2023

这个默认配置格式的初衷是什么?这么改,后续我们如果切换默认配置格式,可能会导致兼容性问题。

@jarily
Copy link
Contributor Author

jarily commented May 23, 2023

我们的配置中心使用的是toml格式,但是配置文件名均没有使用.toml后缀,如果业务来调整不太现实,希望框架能支持一下没有后缀名时默认为toml格式

@askuy
Copy link
Contributor

askuy commented May 23, 2023

我们的配置中心使用的是toml格式,但是配置文件名均没有使用.toml后缀,如果业务来调整不太现实,希望框架能支持一下没有后缀名时默认为toml格式

如果加了没有后缀的,那么破坏了兼容性,因为以前没有配置后缀会报错。也不利于排查用户配置错了,导致的问题排查。

建议搞个环境变量。类似于export EmptyConfigExt=".toml",或者export EmptyConfigExt=".yaml",或者export EmptyConfigExt=""。

func extParser(configAddr string) econf.ConfigType {
	ext := filepath.Ext(configAddr)
        // 说明用户设置了没有后缀名,默认配置选择哪种类型 
        // 并且这个时候,后缀名也为空,那么选用用户设置的默认配置
        // 如果用户没有选择默认配置,立刻报错 
        if os.Getenv("EmptyConfigExt") != ""  && ext == ""{
           ext = os.Getenv("EmptyConfigExt")
        }
	switch ext {
	case ".json":
		return econf.ConfigTypeJSON
	case ".toml":
		return econf.ConfigTypeToml
	case ".yaml":
		return econf.ConfigTypeYaml
	default:
		elog.EgoLogger.Panic("data source: invalid configuration type")
	}
	return ""
}

建议以上方式改,用户也可以扩展自己的默认配置是什么,看以上方案是否能解决你问题,如果可以的话,在看环境变量名字怎么定

@jarily
Copy link
Contributor Author

jarily commented May 24, 2023

我们的配置中心使用的是toml格式,但是配置文件名均没有使用.toml后缀,如果业务来调整不太现实,希望框架能支持一下没有后缀名时默认为toml格式

如果加了没有后缀的,那么破坏了兼容性,因为以前没有配置后缀会报错。也不利于排查用户配置错了,导致的问题排查。

建议搞个环境变量。类似于export EmptyConfigExt=".toml",或者export EmptyConfigExt=".yaml",或者export EmptyConfigExt=""。

func extParser(configAddr string) econf.ConfigType {
	ext := filepath.Ext(configAddr)
        // 说明用户设置了没有后缀名,默认配置选择哪种类型 
        // 并且这个时候,后缀名也为空,那么选用用户设置的默认配置
        // 如果用户没有选择默认配置,立刻报错 
        if os.Getenv("EmptyConfigExt") != ""  && ext == ""{
           ext = os.Getenv("EmptyConfigExt")
        }
	switch ext {
	case ".json":
		return econf.ConfigTypeJSON
	case ".toml":
		return econf.ConfigTypeToml
	case ".yaml":
		return econf.ConfigTypeYaml
	default:
		elog.EgoLogger.Panic("data source: invalid configuration type")
	}
	return ""
}

建议以上方式改,用户也可以扩展自己的默认配置是什么,看以上方案是否能解决你问题,如果可以的话,在看环境变量名字怎么定

这个方案挺好,代码已经push,麻烦重新CR一下

@codecov
Copy link

codecov bot commented May 24, 2023

Codecov Report

Merging #330 (06ef69e) into master (3cbcc35) will increase coverage by 0.09%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #330      +/-   ##
==========================================
+ Coverage   57.19%   57.29%   +0.09%     
==========================================
  Files          81       81              
  Lines        3077     3077              
==========================================
+ Hits         1760     1763       +3     
+ Misses       1148     1147       -1     
+ Partials      169      167       -2     
Flag Coverage Δ
unittests 57.29% <ø> (+0.09%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 2 files with indirect coverage changes

@askuy askuy merged commit aba722c into gotomicro:master May 24, 2023
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.

3 participants