search

bithavoc / express-winston

express.js middleware for winstonjs
https://www.npmjs.com/package/express-winston
MIT License
796 stars 187 forks source link

Enhance "metaField" and add "requestField and "responseField" options #222

Closed yinzara closed 4 years ago

yinzara commented 4 years ago

This PR was created primarily for full support of using this library with @google-cloud/logging-winston on Google Cloud Logging (StackDriver). Google has requirements about where an error stack trace and http request must be stored in the log entry and the current express-winston library does not allow it.

This PR includes a single BREAKING CHANGE related to the "metaField" option. See Readme for details of upgrade requirements.

This PR adds/fixes the following functionality (all summed up in the Readme).

  1. "metaField" now allows storing metadata at log entry root by setting its value to "null"
  2. added "requestField" and "responseField" options
  3. fix implementation of "meta" option in the errorLogger function
  4. Added .editorconfig which matched best to the current style and performed a code reformat of the 'test.js' and 'index.js'
  5. Add TypeScript typings to the library so it isn't necessary to add a devDependency to @types/express-winston
bithavoc commented 4 years ago

@yinzara also please add a record to Changelog so we can merge it ASAP once the comments are resolved.

yinzara commented 4 years ago

I updated the changelog and the readme related to the best practice for Google Cloud logging (I actually used it in my own installation to make sure it was good).

yinzara commented 4 years ago

@bithavoc Can I get this approved and merged? I want to use it in my own projects and hate running a custom version of a library ;-)

yinzara commented 4 years ago

@bithavoc I've rebased against master since there was another release. Can we get this merged or review of additional changes necessary?

bithavoc commented 4 years ago

@yinzara good stuff, I'll merge and release later today, first I need to deploy the 3.4.

bithavoc commented 4 years ago

crap, @yinzara we have conflicts again.

yinzara commented 4 years ago

It was just the app version and changelog. I've merged again and updated.

yinzara commented 4 years ago

Merging now! Didn't see I had already been given access.

yinzara commented 4 years ago

So I've merged and tagged it but there isn't an automated build and release to NPM. I'm guessing you have to do that? My NPM account is also "yinzara" if you want to give it access, I can publish a new version but totally up to you.

yinzara commented 4 years ago

It wouldn't be a bad idea for us to update the .travis.yml to deploy to GitHub releases and to NPM on every tag. Save you a lot of pain.

bithavoc commented 4 years ago

How can I restrict it so I can be the only one creating releases? I just don't want the releases published to NPM directly from Github, I need to check what we publish you know, see https://github.com/bithavoc/express-winston/issues/192#issuecomment-441773744

yinzara commented 4 years ago

I had to look this up.

So you can require that Pull Requests all must have an Approval by the project owner before they can be merged to master.

https://help.github.com/en/articles/enabling-required-reviews-for-pull-requests

You can then tie the build step that publishes to NPM to only the master branch.

You can enable "Protected Branches" with specific status checks that have to pass as well if you want to enforce other stuff (like updating version numbers).

bithavoc commented 4 years ago

so then we can have a branch releases that auto publishes to NPM but I'm the only that approves what gets merged. Is there an integration we activate for NPM publishing?

I'd like us to move away from Travis first, I don't like it anymore after they got acquired. Maybe Github actions or CircleCI?