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
[#175119553] Centralize environment variables in single config module #110
Conversation
Affected stories
New dependencies added: dotenvAuthor: Unknown Description: Loads environment variables from .env file Homepage: https://github.com/motdotla/dotenv#readme
|
Codecov Report
@@ Coverage Diff @@
## master #110 +/- ##
==========================================
- Coverage 84.43% 84.42% -0.01%
==========================================
Files 24 25 +1
Lines 713 732 +19
Branches 76 77 +1
==========================================
+ Hits 602 618 +16
- Misses 106 109 +3
Partials 5 5
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. Moreover the title of the PR should be more descriptive ( something like "Centralize Env variables in single config module" )
Codecov Report
@@ Coverage Diff @@
## master #110 +/- ##
==========================================
+ Coverage 84.43% 85.11% +0.68%
==========================================
Files 24 31 +7
Lines 713 766 +53
Branches 76 81 +5
==========================================
+ Hits 602 652 +50
- Misses 106 109 +3
Partials 5 5
Continue to review full report at Codecov.
|
Co-authored-by: Daniele Manni <BurnedMarshal@users.noreply.github.com>
lgtm besides of the use of env variables in unit tests. I see this is needed in some cases because test suites import the whole function's I see those are legacy problems, maybe we can take advantage to solve them by refactor such functions. The scope is: to only test |
that's right, we can do it with another PR |
StoreSpidLogs/__test__/index.test.ts
Outdated
@@ -92,6 +79,7 @@ describe("StoreSpidLogs", () => { | |||
} | |||
}; | |||
const blobItem = await index(mockedContext as any, aSpidMsgItem); | |||
console.log(blobItem); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
woops
@@ -32,6 +32,7 @@ | |||
"@types/nodemailer": "^4.6.8", | |||
"danger": "^4.0.2", | |||
"danger-plugin-digitalcitizenship": "^0.3.1", | |||
"dotenv": "^8.2.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a question, does dotenv configured in jest.config.js read env.example
by default ?
I know it reads .env
by default but maybe env.example is included as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's not, we might specify it explicitly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the correct file name is .env
.
There is a comment in the first line of this file that indicates to rename, the idea is to indicate that it is just a test file.
@infantesimone I see there are lint issues. No problem, you can fix them and push again. Try https://github.com/pagopa/git-hooks to prevent yourself from pushing bad code and save a roundtrip ;)
|
Co-authored-by: danilo spinelli <danilo.spinelli@pagopa.itt>
lgtm, why still draft? |
This PR introduces the utils/config module which aims to be the unique interface for application configuration.
Every place in which the application used to directly access env variable has been refactored to use this module instead.