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

Add platform agnostic way to read environment variables #1191

Merged
merged 6 commits into from
Jun 26, 2020

Conversation

obecny
Copy link
Member

@obecny obecny commented Jun 12, 2020

Which problem is this PR solving?

Short description of the changes

  • It adds one function to read automatically environment variables without worrying about platform and defaults.

@obecny obecny self-assigned this Jun 12, 2020
@obecny obecny added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. enhancement New feature or request labels Jun 12, 2020
@obecny obecny changed the title Adding platform specific class for getting environment variables Add platform agnostic way to read environment variables Jun 12, 2020
@codecov
Copy link

codecov bot commented Jun 12, 2020

Codecov Report

Merging #1191 into master will increase coverage by 0.00%.
The diff coverage is 93.33%.

@@           Coverage Diff           @@
##           master    #1191   +/-   ##
=======================================
  Coverage   93.29%   93.29%           
=======================================
  Files         124      126    +2     
  Lines        3567     3597   +30     
  Branches      715      724    +9     
=======================================
+ Hits         3328     3356   +28     
- Misses        239      241    +2     
Impacted Files Coverage Δ
...ckages/opentelemetry-core/src/utils/environment.ts 92.30% <92.30%> (ø)
...pentelemetry-core/src/platform/node/environment.ts 100.00% <100.00%> (ø)

@obecny obecny requested review from dyladan and markwolff June 15, 2020 12:12
@obecny
Copy link
Member Author

obecny commented Jun 23, 2020

@open-telemetry/javascript-maintainers @open-telemetry/javascript-approvers ^^

* Gets the environment variables
*/
export function getEnv(): ENVIRONMENT {
const globalEnv = parseEnvironment((window as unknown) as ENVIRONMENT_MAP);
Copy link
Member

Choose a reason for hiding this comment

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

instead of asserting window twice can we do const _window = window as typeof window & ENVIRONMENT_MAP like we did in the API package for global?

Copy link
Member

Choose a reason for hiding this comment

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

ENVIRONMENT_MAP should be made partial. The types will not force you to check if a value exists in this current setup.

Copy link
Member Author

Choose a reason for hiding this comment

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

not sure if I understand correctly your last sentence about partial ?

@dyladan dyladan added the Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) label Jun 26, 2020
@mayurkale22 mayurkale22 merged commit e19a0db into open-telemetry:master Jun 26, 2020
@obecny obecny deleted the environment branch July 8, 2020 12:15
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
* fix(restify): upgrade package to fix type problem

* fix(restify): updated so types are not leaking through open-telemetry#1132

* fix(restify): lint fix

* fix(restify): final fix up for linter

* style: avoid checking in manually edited package.json

* test: require restify again for every test

* test: differenciate between thorwing and failing gracefully

* test: use custom error

* feat: support restify@5

* feat: support restify@7

* feat: support restify@8

* test: cleanup

* test: anyfy arguments on the anonymous handler

* chore: add tav + update supported versions

* fix: turn off tests on node@18 which is not supported for restify

* refactor: remove the import to after setting up instrumentation

Co-authored-by: jscherer92 <jscherer92@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add platform agnostic way to read environment variables
7 participants