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.

Sign up for GitHub

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

Jump to bottom

WIP: Added MongooseHealthIndicator #36

Closed
wants to merge 13 commits into from
Closed

WIP: Added MongooseHealthIndicator #36

wants to merge 13 commits into from

Conversation

nartc
Copy link

@nartc nartc commented Jan 7, 2019 •
edited
Loading

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Added MongooseHealthIndicator
  • Brought DatabasePingCheckSettings interface from DatabaseHealthIndicator out to its own file
  • Refactored HealthIndicator abstract class to contain more common functionalities to share between MongooseHealthIndicator and DatabaseHealthIndicator
  • DatabaseHealthIndicator and MongooseHealthIndicator are much cleaner since the exposed method are common. The only difference (abstract method) is the different method to ping the Connection based on Mongoose or TypeOrm.
  • Added mongoose and @nestjs/mongooses to peerDependencies, added @types/mongoose to devDependencies
  • Added MongooseHealthIndicator usage to the sample application. Also updated package.json in the sample app to have Mongoose installed.
[ ] Bugfix
[x] Feature
[x] Code style update (formatting, local variables)
[x] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior?

MongooseHealthCheck is missing
Issue Number: #35

What is the new behavior?

MongooseHealthCheck is added

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

Please review as it contains more changes than intended.

  • DatabasePingCheckOptions now takes in a generic of type Connection | MongooseConnection
  • HealthCheckIndicator now takes in a generic of type Connection | MongooseConnection

EDIT: Please see comment below

Added MongooseHealthIndicator
3bdca98 
- Brought DatabasePingCheckSettings interface from DatabaseHealthIndicator out to its own file
- Refactored HealthIndicator abstract class to contain more common functionalities to share between MongooseHealthIndicator and DatabaseHealthIndicator
- DatabaseHealthIndicator and MongooseHealthIndicator are much cleaner since the exposed method are common. The only difference (abstract method) is the different method to ping the Connection based on Mongoose or TypeOrm.
- Added mongoose and @nestjs/mongooses to peerDependencies, added @types/mongoose to devDepdencies
- Added MongooseHealthIndicator usage to the sample application. Also updated package.json in the sample app to have Mongoose installed.
@nartc nartc closed this Jan 7, 2019
@nartc
Copy link
Author

nartc commented Jan 7, 2019

Overlooked DnsHealthIndicator. Will fix. Closed for now

Fixed HealthIndicator abstract for DnshealthIndicator
583ec7b 
- Reverted HealthIndicator
- Added DatabaseHealthIndicator abstract extends HealthIndicator and added common functionalities for DatabaseHealthIndicator
- Renamed DatabaseHealthIndicator to TypeOrmHealthIndicator to differentiate between TypeOrm and MongooseHealthIndicator
- Removed generic from HealthIndicator and DatabasePingCheckSettings. Used union type Connection | MongooseConnection
- Updated TermninusCoreModule
@nartc nartc reopened this Jan 7, 2019
@nartc
Copy link
Author

nartc commented Jan 7, 2019

  • Reverted HealthIndicator
  • Added DatabaseHealthIndicator abstract extends HealthIndicator and added common functionalities for DatabaseHealthIndicator
  • Renamed the "old" DatabaseHealthIndicator to TypeOrmHealthIndicator to differentiate between TypeOrm and MongooseHealthIndicator
  • Removed generic from HealthIndicator and DatabasePingCheckSettings. Used union type Connection | MongooseConnection
  • Updated TermninusCoreModule

@nartc nartc closed this Jan 7, 2019
@nartc nartc reopened this Jan 7, 2019
@nartc nartc closed this Jan 7, 2019
@nartc
Copy link
Author

nartc commented Jan 7, 2019

Closed due to Unit Test failing.

@nartc nartc reopened this Jan 7, 2019
@nartc nartc closed this Jan 7, 2019
@nartc nartc reopened this Jan 7, 2019
@nartc
Copy link
Author

nartc commented Jan 7, 2019

Closed. Can't seem to connect to MongoDB for Unit Test.

This PR has been flooded with so many meaningless comment so Imma close and will re-submit another PR.

@nartc nartc closed this Jan 7, 2019
@nartc nartc reopened this Jan 7, 2019
@nartc nartc closed this Jan 7, 2019
BrunnerLivio
BrunnerLivio reviewed Jan 7, 2019
View reviewed changes
lib/health-indicators/typeorm/typeorm.health.ts Show resolved Hide resolved
sample/000-dogs-app/src/health.module.ts

const getTerminusOptions = (
dogHealthIndicator: DogHealthIndicator,
catHealthIndicator: CatHealthIndicator,
dnsHealthIndicator: DNSHealthIndicator,
mongoHealthIndicator: MongooseHealthIndicator,
Copy link
Member

Choose a reason for hiding this comment

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

We should keep the sample dogs sample app as it is. It is useful for the user to start the sample app, without having to start a MongoDB instance locally.
If you want to create a sample, feel free to create a new folder sample/001-mongoose.

Copy link
Author

Choose a reason for hiding this comment

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

Done

@BrunnerLivio
Copy link
Member

Looking good! :)

This PR has been flooded with so many meaningless comment so Imma close and will re-submit another PR.

You can also squash these commits. Try out git rebase -i @~12 and then force push on your branch.

While you are rebasing, could you use this commit format? Note: also use present tense for commits (e.g. feat(health): Add MongooseHealthIndicator)

@nartc nartc mentioned this pull request Jan 8, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BrunnerLivio BrunnerLivio BrunnerLivio left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@nartc @BrunnerLivio