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

chore: Update environment variable seperator comment to indicate double underscore #164

Merged
merged 1 commit into from
Sep 5, 2023

Conversation

bgins
Copy link
Contributor

@bgins bgins commented Sep 5, 2023

Description

This PR makes the following changes:

  • Update environment variable separator to one underscore It should be a double underscore!
  • Update comment to indicate a double underscore should be used

Type of change

  • Documentation

Test plan (required)

No test, check the updated comment.

@bgins bgins requested a review from a team as a code owner September 5, 2023 17:49
@QuinnWilton
Copy link

This LGTM! @zeeshanlakhani was there a reason you'd done two underscores originally? I'd noticed that happening in the fission-server too, and wasn't sure whether that was an intentional choice.

@bgins
Copy link
Contributor Author

bgins commented Sep 5, 2023

@QuinnWilton was just chatting with @zeeshanlakhani. Turns out it should be double underscores, the comment is wrong. 🙃

Updating the PR to fix the comment.

@zeeshanlakhani
Copy link
Contributor

This LGTM! @zeeshanlakhani was there a reason you'd done two underscores originally? I'd noticed that happening in the fission-server too, and wasn't sure whether that was an intentional choice.

@QuinnWilton, after talking to @bgins, it should be two underscores, but the comment is wrong :).

Example (overriding toml configuration):

[server]
environment = "local"
metrics_port = 4000
port = 3030

To override environment, you'd use APP__SERVER__ENVIRONMENT="dev". Why 2 > 1? Well, metrics port would be APP__SERVER__METRICS_PORT=2222

@bgins bgins force-pushed the bgins/fix-env-var-underscore branch from 25ba916 to 0399c10 Compare September 5, 2023 19:10
@bgins bgins force-pushed the bgins/fix-env-var-underscore branch from 0399c10 to 7cecd3c Compare September 5, 2023 19:11
@bgins bgins changed the title fix: Update environment variable seperator to one underscore chore: Update environment variable seperator comment to indicate double underscore Sep 5, 2023
@zeeshanlakhani zeeshanlakhani merged commit 20c981f into main Sep 5, 2023
23 checks passed
@zeeshanlakhani zeeshanlakhani deleted the bgins/fix-env-var-underscore branch September 5, 2023 19:24
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