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.

Sign up for GitHub

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

Change environment prefix from nms to nginx_agent #706

Merged
merged 11 commits into from
Jun 17, 2024
Merged

Change environment prefix from nms to nginx_agent #706

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
5b8a8dc
changed env variable prefix from NMS to NGINX_AGENT
spencerugbo Jun 13, 2024
1997ecd
changed env variable prefix from NMS to NGINX_AGENT
spencerugbo Jun 13, 2024
3c5157c
renamed env prefix constants
spencerugbo Jun 17, 2024
aa1a5ea
renamed env prefix constants
spencerugbo Jun 17, 2024
5ff0aa5
refactored string building of oldKey and newKey
spencerugbo Jun 17, 2024
8873427
refactored string building of oldKey and newKey
spencerugbo Jun 17, 2024
596b764
added test for env prefix migration
spencerugbo Jun 17, 2024
f1d4a55
added test for env prefix migration
spencerugbo Jun 17, 2024
e4e8ca6
refactored string builing of oldKey and newKey
spencerugbo Jun 17, 2024
c3c01d3
refactored string builing of oldKey and newKey
spencerugbo Jun 17, 2024
a31da69
fixed deprecated env prefix migration test
spencerugbo Jun 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,10 @@ func main() {
defer logFile.Close()
}

if config.MigratedEnv {
log.Warnf("The environment variable prefix 'NMS' is deprecated. Prefix has been migrated to 'NGINX_AGENT'. Please update your configuration to use the new prefix.")
}

log.Tracef("Config loaded from disk, %v", loadedConfig)

if loadedConfig.DisplayName == "" {
Expand Down
18 changes: 16 additions & 2 deletions src/core/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,10 @@ const (
`
)

var Viper = viper.NewWithOptions(viper.KeyDelimiter(agent_config.KeyDelimiter))
var (
Viper = viper.NewWithOptions(viper.KeyDelimiter(agent_config.KeyDelimiter))
MigratedEnv = false
)

func SetVersion(version, commit string) {
ROOT_COMMAND.Version = version + "-" + commit
Expand Down Expand Up @@ -133,7 +136,7 @@ func deprecateFlags() {
}

func RegisterFlags() {
Viper.SetEnvPrefix(EnvPrefix)
Viper.SetEnvPrefix(NewEnvPrefix)
Viper.SetEnvKeyReplacer(strings.NewReplacer("-", "_"))
Viper.AutomaticEnv()

Expand All @@ -149,6 +152,17 @@ func RegisterFlags() {
if err := Viper.BindPFlag(strings.ReplaceAll(flag.Name, "-", "_"), fs.Lookup(flag.Name)); err != nil {
return
}

oldKey := strings.ToUpper(OldEnvPrefix + "_" + strings.ReplaceAll(flag.Name, "-", "_"))
newKey := strings.ToUpper(NewEnvPrefix + "_" + strings.ReplaceAll(flag.Name, "-", "_"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally it's good practice to use fmt.Sprintf() for building strings instead of "string" + "another_string". However, I think your solution is more readable so it's fine here.

I would perhaps reuse theagent_config.KeyDelimiter variable we have here:

Suggested change
oldKey := strings.ToUpper(OldEnvPrefix + "_" + strings.ReplaceAll(flag.Name, "-", "_"))
newKey := strings.ToUpper(NewEnvPrefix + "_" + strings.ReplaceAll(flag.Name, "-", "_"))
oldKey := strings.ToUpper(
OldEnvPrefix + agent_config.KeyDelimiter + strings.ReplaceAll(flag.Name, "-", agent_config.KeyDelimiter)
)
newKey := strings.ToUpper(
NewEnvPrefix + agent_config.KeyDelimiter + strings.ReplaceAll(flag.Name, "-", agent_config.KeyDelimiter)
)


if os.Getenv(oldKey) != "" && os.Getenv(newKey) == "" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the (admittedly unlikely) case where both oldKey and newKey are set?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If both oldKey and newKey are set we assume the newKey is the one with the correct value and ignore the value from the old key

if err := os.Setenv(newKey, os.Getenv(oldKey)); err != nil {
log.Warnf("Failed to set environment variable %s: %v", newKey, err)
}
MigratedEnv = true
}

err := Viper.BindEnv(flag.Name)
if err != nil {
log.Warnf("Error occurred binding env %s: %v", flag.Name, err)
Expand Down
2 changes: 1 addition & 1 deletion src/core/config/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -480,7 +480,7 @@ func TestUpdateAgentConfig(t *testing.T) {
}

func setEnvVariable(t *testing.T, name string, value string) {
key := strings.ToUpper(EnvPrefix + agent_config.KeyDelimiter + name)
key := strings.ToUpper(NewEnvPrefix + agent_config.KeyDelimiter + name)
err := os.Setenv(key, value)
require.NoError(t, err)
}
Expand Down
3 changes: 2 additions & 1 deletion src/core/config/defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,8 @@ const (
DynamicConfigFileAbsFreeBsdPath = "/var/db/nginx-agent/agent-dynamic.conf"
ConfigFileName = "nginx-agent.conf"
ConfigFileType = "yaml"
EnvPrefix = "nms"
OldEnvPrefix = "nms"
NewEnvPrefix = "nginx_agent"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think OldEnvPrefix should rather be called LegacyEnvPrefix and NewEnvPrefix just EnvPrefix. That way we could eventually get rid of LegacyEnvPrefix and we'd be left with EnvPrefix.

ConfigPathKey = "path"
DynamicConfigPathKey = "dynamic_config_path"

Expand Down
18 changes: 16 additions & 2 deletions test/integration/vendor/github.com/nginx/agent/v2/src/core/config/config.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion test/integration/vendor/github.com/nginx/agent/v2/src/core/config/defaults.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

18 changes: 16 additions & 2 deletions test/performance/vendor/github.com/nginx/agent/v2/src/core/config/config.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion test/performance/vendor/github.com/nginx/agent/v2/src/core/config/defaults.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading