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

adding option to set the DB name #205

Merged
merged 2 commits into from
Sep 19, 2022

Conversation

millerdk12
Copy link
Contributor

Adding support for using a different DB name than the default one from the connection URI.

The use case is to allow users to use log with a pre-connected mongo client, but use a different name than the default.

@@ -14,6 +14,8 @@ const MongoDB = require('../lib/winston-mongodb').MongoDB;

const dbUrl = process.env.USER_WINSTON_MONGODB_URL
||process.env.WINSTON_MONGODB_URL||'mongodb://localhost:27017/winston';
const dbName = process.env.WINSTON_MONGODB_DBNAME
||'otherWinston';
Copy link

Choose a reason for hiding this comment

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

What's the reasoning behind 'otherWinston' here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just an example database name for the test. So checking the results there should be two databases logged to 'winston' and 'otherWinston'.

@wbt
Copy link

wbt commented Sep 19, 2022

Do you think this is finished, tested, and good to merge?

@millerdk12
Copy link
Contributor Author

Do you think this is finished, tested, and good to merge?

Yes, it is finished. I have tested it using the unit tests from this project, and I also tried it out in my app using a npm package link and it is working as expected.

@wbt
Copy link

wbt commented Sep 19, 2022

On second note, I don't have permission to do a release of this on npm, and that might be for the better as I don't currently use the mongodb transport in any of my own projects. I don't know if there are other project maintainers who can give it a good look with testing and merge, but on a quick review of the code the PR seems reasonable to me.

@millerdk12
Copy link
Contributor Author

Okay no problem, are you able to request one of the maintainers to take a look?

@wbt
Copy link

wbt commented Sep 19, 2022

I don't actually know who all has that permission on this project.

@millerdk12
Copy link
Contributor Author

Looking through the git history, it looks like @yurijmikhalevich has the access. Yuri, are you able to provide a review on this one?

Copy link
Contributor

@DABH DABH left a comment

Choose a reason for hiding this comment

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

LGTM (I think I can merge and release on npm…)

@DABH DABH merged commit 4702ba3 into winstonjs:master Sep 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants