-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Winston 3 upgrade #5496
Winston 3 upgrade #5496
Conversation
* ⚡ Release 3.1.3 * Update CHANGELOG.md
Bumps [winston](https://github.com/winstonjs/winston) and [winston-daily-rotate-file](https://github.com/winstonjs/winston-daily-rotate-file). These dependencies needed to be updated together. Updates `winston` from 2.4.4 to 3.1.0 - [Release notes](https://github.com/winstonjs/winston/releases) - [Changelog](https://github.com/winstonjs/winston/blob/master/CHANGELOG.md) - [Commits](winstonjs/winston@2.4.4...3.1.0) Updates `winston-daily-rotate-file` from 1.7.2 to 3.5.1 - [Release notes](https://github.com/winstonjs/winston-daily-rotate-file/releases) - [Commits](winstonjs/winston-daily-rotate-file@v1.7.2...v3.5.1) Signed-off-by: dependabot[bot] <[email protected]>
Bumps [winston](https://github.com/winstonjs/winston) and [winston-daily-rotate-file](https://github.com/winstonjs/winston-daily-rotate-file). These dependencies needed to be updated together. Updates `winston` from 2.4.4 to 3.2.0 - [Release notes](https://github.com/winstonjs/winston/releases) - [Changelog](https://github.com/winstonjs/winston/blob/master/CHANGELOG.md) - [Commits](winstonjs/winston@2.4.4...3.2.0) Updates `winston-daily-rotate-file` from 1.7.2 to 3.6.0 - [Release notes](https://github.com/winstonjs/winston-daily-rotate-file/releases) - [Commits](winstonjs/winston-daily-rotate-file@v1.7.2...v3.6.0) Signed-off-by: dependabot[bot] <[email protected]>
# Conflicts: # package-lock.json # package.json
# Conflicts: # package-lock.json
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.
This is great.
The failing tests are a problem that needs to be researched and may be a byproduct of dependency upgrades
2) MongoStorageAdapter auto-escapes symbols in auth information
- Unhandled promise rejection: TypeError: Cannot read property 's' of null
3) MongoStorageAdapter doesn't double escape already URI-encoded information
- Unhandled promise rejection: TypeError: Cannot read property 's' of null
4) MongoStorageAdapter preserves replica sets
- Unhandled promise rejection: TypeError: Cannot read property 's' of null
I rebased locally to get other dependency changes and I still get the above.
}); | ||
_.remove(additionalTransports, transport => { | ||
return transport.name === transportName; | ||
const matchingTransport = logger.transports.find(t1 => { |
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.
do we have to worry about if there are multiple transport of a particular kind? ie two file transports, one for error and one for info? I'm not sure.
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 I think you are right, the code has changed so now it might be possible to have two transports with the same name.
I think previously this code prevented the above scenario.
additionalTransports.forEach(transport => {
transports[transport.name] = transport;
});
I'll investigate...
src/Adapters/Logger/WinstonLogger.js
Outdated
delete transports['parse-server-error']; | ||
} else if (!_.isUndefined(options.dirname)) { | ||
transports['parse-server'] = new DailyRotateFile( | ||
if (!_.isUndefined(options.dirname) && !_.isNull(options.dirname)) { |
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.
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.
No worries, i'll update to use isNil
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.
Thanks @acinader, I'll investigate the failing tests.
I think tests started failing since a jasmine upgrade from 3.1.0 to 3.4.0 via this commit e396612. Downgrading to 3.1.0 fixes the breaking tests. I'll update failing tests to set up a spy with a return value that won't throw exceptions so that we don't have to downgrade jasmine. |
Codecov Report
@@ Coverage Diff @@
## master #5496 +/- ##
==========================================
- Coverage 94.04% 93.98% -0.06%
==========================================
Files 125 125
Lines 9097 9092 -5
==========================================
- Hits 8555 8545 -10
- Misses 542 547 +5
Continue to review full report at Codecov.
|
@stage88 I didn't realize that jasmine has been breaking the tests since its last upgrade. I am going to take a look at the release notice to understand what is going on and I will also look at your fix. See #5500 Will get this merged in after that. This looks great. Big thanks for undertaking this chore! |
Should I try and fix the code as per your comment above: #5496 (comment)? I should be able to have it done tonight ... |
fixes: #5275 |
* ⚡ Release 3.1.3 (parse-community#5267) * ⚡ Release 3.1.3 * Update CHANGELOG.md * ⬆️ Bump winston and winston-daily-rotate-file Bumps [winston](https://github.com/winstonjs/winston) and [winston-daily-rotate-file](https://github.com/winstonjs/winston-daily-rotate-file). These dependencies needed to be updated together. Updates `winston` from 2.4.4 to 3.1.0 - [Release notes](https://github.com/winstonjs/winston/releases) - [Changelog](https://github.com/winstonjs/winston/blob/master/CHANGELOG.md) - [Commits](winstonjs/winston@2.4.4...3.1.0) Updates `winston-daily-rotate-file` from 1.7.2 to 3.5.1 - [Release notes](https://github.com/winstonjs/winston-daily-rotate-file/releases) - [Commits](winstonjs/winston-daily-rotate-file@v1.7.2...v3.5.1) Signed-off-by: dependabot[bot] <[email protected]> * Rewrote WinstonLogger to work with winston 3.x api * Changed winston logger test to use winston-transport * Added winston-transport dependency * Close and remove transports before adding them again * Changed to strict equal * Override adapter name * Updated and added getLogs tests * Bump winston and winston-daily-rotate-file Bumps [winston](https://github.com/winstonjs/winston) and [winston-daily-rotate-file](https://github.com/winstonjs/winston-daily-rotate-file). These dependencies needed to be updated together. Updates `winston` from 2.4.4 to 3.2.0 - [Release notes](https://github.com/winstonjs/winston/releases) - [Changelog](https://github.com/winstonjs/winston/blob/master/CHANGELOG.md) - [Commits](winstonjs/winston@2.4.4...3.2.0) Updates `winston-daily-rotate-file` from 1.7.2 to 3.6.0 - [Release notes](https://github.com/winstonjs/winston-daily-rotate-file/releases) - [Commits](winstonjs/winston-daily-rotate-file@v1.7.2...v3.6.0) Signed-off-by: dependabot[bot] <[email protected]> * Fixed tests, updated parse logging * Fixed tests, better error logging * Fix failing tests * Updates as per review
There are 5 tests failing, however I don't think these are related.