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

feat(rabbitmq): add custom logger to configuration #401

Merged

Conversation

AdrienEtienne
Copy link
Contributor

Description

This PR is about adding a custom logger, implementing the LoggerService interface from NestJS.

What is done

We pass the logger directly into the configuration object.

Missing \ Improvements

Currently the class RabbitMQModule is still using the default logger. I didn't found a way to pass a custom logger from the configuration. Help would be appreciated :).

Related PRs

This PR is about allowing to set log level to verbose: #264

I think it is a better solution to just pass a custom logger to the module.

@underfisk
Copy link
Contributor

LGTM, @WonderPanda Any thoughts on this?

@AdrienEtienne
Copy link
Contributor Author

Any update on that?

@WonderPanda
Copy link
Collaborator

All the integration tests seem to be failing, we'll need to look into this before the PR can be merged

@AdrienEtienne
Copy link
Contributor Author

@WonderPanda do you have any idea why the tests are failing?

@AdrienEtienne
Copy link
Contributor Author

A new e2e test I wrote was failing... I changed that.

@underfisk
Copy link
Contributor

@AdrienEtienne The tests seem to be stable and pass now, can you just check the unsafety that's reported on CI nodejs 17? Once that passes, I'm okay to merge

@AdrienEtienne
Copy link
Contributor Author

@AdrienEtienne The tests seem to be stable and pass now, can you just check the unsafety that's reported on CI nodejs 17? Once that passes, I'm okay to merge

Ok thanks.

The tests are failing randomly when I run those locally. I changed the test on the custom logger, got no error with node v17.
But still have tests failing sometimes, no matter which version of node I am using.

@underfisk underfisk merged commit 242fc69 into golevelup:master Apr 21, 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