-
Notifications
You must be signed in to change notification settings - Fork 157
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
Pass 'audit_hash_type' option for getStream method #344
Conversation
daily-rotate-file.js
Outdated
@@ -91,7 +91,8 @@ var DailyRotateFile = function (options) { | |||
extension: options.extension ? options.extension : '', | |||
create_symlink: options.createSymlink ? options.createSymlink : false, | |||
symlink_name: options.symlinkName ? options.symlinkName : 'current.log', | |||
watch_log: options.watchLog ? options.watchLog : false | |||
watch_log: options.watchLog ? options.watchLog : false, | |||
audit_hash_type: options.auditHashType || 'sha256' |
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 was not sure about default value to be used for audit_hash_type option. Hence used 'sha256' as it is mostly recommended and considered as strong hashing algorithm.
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.
Seems like a sensible default, but I'd recommend matching the default-setting format of the others, for consistency and to help avoid type confusion (about the boolean-type return value of a boolean OR operation).
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 for the comment!! Updated it as current default-setting format
Used Ternary operator to set option value instead of OR condition
Merged, but I don't have permission to publish on npm. @DABH might be able to change that, or @mattberther might be able to publish. |
@wbt Ah, I could add you on GitHub, but @mattberther is the only one with permissions on NPM. @mattberther , perhaps you could add me and @wbt on NPM? (We seem to be the two primary Winston maintainers right now…) |
On this publish, I also somewhat defer to @mattberther for making release notes as the GitHub release tags haven't been kept up with npm releases, and there's no visible changelog to know when to go back to. |
@mattberther Don’t be humble, you have been a fantastic maintainer for this project over the years :) I can’t commit to any help beyond possibly doing some releases sometimes, but if that would ease your maintenance burden at all, would be happy to volunteer (though I have to caveat that my non-existent free time is already spread a bit thin with Winston and colors.js) |
Basically the same as what @DABH said, but with the further caveat that I would prefer deferring to you for the reasons noted above. Somehow (pretty easy to see how, actually), when I became a committer on winston, I wound up getting notifications for all the issues on winston-daily-rotate file as well. |
Adding 'audit_hash_type' option while using .getStream to avoid unsupported digest method issue faced on SLES platform.
Please check the issue : #340