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

Change environment prefix from nms to nginx_agent #706

Merged
merged 11 commits into from
Jun 17, 2024

Conversation

spencerugbo
Copy link
Contributor

Proposed changes

Agent now uses NGINX_AGENT as its environment prefix as opposed to NMS. If the old prefix is used, Agent migrates the value to the correct key and a message is logged warning the user to switch to the new prefix

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING document
  • I have run make install-tools and have attached any dependency changes to this pull request
  • If applicable, I have added tests that prove my fix is effective or that my feature works
  • If applicable, I have checked that any relevant tests pass after adding my changes
  • If applicable, I have updated any relevant documentation (README.md)
  • If applicable, I have tested my cross-platform changes on Ubuntu 22, Redhat 8, SUSE 15 and FreeBSD 13

@spencerugbo spencerugbo self-assigned this Jun 13, 2024
@github-actions github-actions bot added bug Something isn't working chore Pull requests for routine tasks dependencies labels Jun 13, 2024
Copy link

netlify bot commented Jun 13, 2024

Deploy Preview for agent-public-docs canceled.

Name Link
🔨 Latest commit a31da69
🔍 Latest deploy log https://app.netlify.com/sites/agent-public-docs/deploys/667033665bd51c00085e48d8

Comment on lines 106 to 107
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.

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

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

@olli-holmala
Copy link
Contributor

Could you add some unit tests for this? I think Go's testing.T should support setting env vars nicely with t.Setenv(), so this should be testable.

Comment on lines 156 to 157
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)
)

@aphralG
Copy link
Contributor

aphralG commented Jun 17, 2024

Will create task to check MigratedEnv in UnitTests in the future, otherwise LGTM

@spencerugbo spencerugbo merged commit 174be3d into main Jun 17, 2024
33 checks passed
@spencerugbo spencerugbo deleted the environment-variables-for-nginx-one branch June 17, 2024 14:40
@dhurley dhurley linked an issue Jun 25, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working chore Pull requests for routine tasks dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Agent uses "nms" as its environment prefix
4 participants