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

refactor(root): introducing graceful shutdown and optimise docker images #6754

Open
wants to merge 22 commits into
base: next
Choose a base branch
from

Conversation

merrcury
Copy link
Member

What changed? Why was the change needed?

This PR linked to below issues

  • Fixes INF-447
  • Fixes INF-443
  • Fixes INF-445

Screenshots

New Images are now 46% Slimmer
Screenshot 2024-10-23 at 3 09 50 PM

merrcury and others added 13 commits October 22, 2024 16:52
Add a new module called "graceful-shutdown" to the "application-generic" library. This module includes a "ShutdownService" class that provides a timeout value for graceful shutdown. The timeout value is retrieved from the environment variable "GRACEFUL_SHUTDOWN_TIMEOUT" with a default value of 5000. This module also exports the "ShutdownService" for external use.
@merrcury merrcury self-assigned this Oct 23, 2024
@merrcury merrcury requested a review from a team as a code owner October 23, 2024 09:40
@merrcury merrcury requested review from SokratisVidros and scopsy and removed request for a team October 23, 2024 09:40
Copy link

netlify bot commented Oct 23, 2024

Deploy Preview for novu-stg-vite-dashboard-poc ready!

Name Link
🔨 Latest commit 4a3e98d
🔍 Latest deploy log https://app.netlify.com/sites/novu-stg-vite-dashboard-poc/deploys/6722a6da0b09f500089bdbf3
😎 Deploy Preview https://deploy-preview-6754--novu-stg-vite-dashboard-poc.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@@ -211,7 +211,6 @@
"devDependencies": {
"@apidevtools/json-schema-ref-parser": "11.6.4",
"@arethetypeswrong/cli": "^0.16.4",
"@nestjs/common": "10.4.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

issue: We need to keep this so that the test suites have the needed types. Please revert the removal.‏

Copy link
Member Author

Choose a reason for hiding this comment

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

Hey @rifont, If I am adding this into dev dependency and then running --prod its not getting installed and API build up is failing. any workaround/idea?

Copy link
Contributor

@rifont rifont Oct 28, 2024

Choose a reason for hiding this comment

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

That's strange, as @novu/api declares @nestjs/common as a dependency here which means it should be installed when using the --prod flag.

Could you please share the error log for further debugging?

I believe reverting the removal of this dependency is a blocker for Framework tests to pass.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hey @rifont, Please find error below:
Screenshot 2024-10-29 at 11 49 50 AM

Copy link
Contributor

Choose a reason for hiding this comment

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

This is now solved. See 4a3e98d.

The trick is that @novu/framework requires a range for @nestjs/common peer dependency but by pinning the @nestjs/common version via PNPM overrides we effectively teach PNPM to add the correct dependencies when installing only production dependencies.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tested locally as well. It worked fabulous for me. Sok, we need an article for all of this 😄 .

Copy link
Contributor

Choose a reason for hiding this comment

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

We will! 😆

Copy link
Contributor

@SokratisVidros SokratisVidros left a comment

Choose a reason for hiding this comment

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

😍

…dation

This commit adds validation for the GRACEFUL_SHUTDOWN_TIMEOUT environment variable in multiple config files. The variable is now validated as a number with a default value of 5000. This ensures that the application gracefully shuts down within the specified timeout period.

Refs: #issue-number
The @nestjs/common global override set to "@nestjs/common@>=10.0.0 <11.0.0": "10.4.1" is necessary to teach the API application to inject it properly to @novu/framework as a peer dependency.

Refer to #6754 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants