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 move inbound mail parser to worker #4986

Merged
merged 10 commits into from
Jan 30, 2024

Conversation

djabarovgeorge
Copy link
Contributor

What change does this PR introduce?

Move the inbound mail parser worker to the worker app.

Why was this change needed?

There is no reason for us to spin the inbound mail parser worker in the API app.

Other information (Screenshots)

@scopsy
Copy link
Contributor

scopsy commented Dec 15, 2023

Just a note, it will require a terraform change aswell* cc @Cliftonz

Copy link
Contributor

@LetItRock LetItRock left a comment

Choose a reason for hiding this comment

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

from the code perspective it looks good to me! 🔥
@Cliftonz we would need to implement appropriate cloud infra changes after this PR is merged

@Cliftonz
Copy link
Contributor

Cliftonz commented Jan 5, 2024

@djabarovgeorge I see the spell check failing for the moment.

however I want to confirm that now the workers will be processing the inbound mail and it will go like:

email -> dns -> inbound-email service-> queue -> email parse (on worker) -> standard queue

If this is true then we can merge this in and the general worker service should be able to handle this.

@djabarovgeorge
Copy link
Contributor Author

email -> dns -> inbound-email service-> queue -> email parse (on worker) -> standard queue

@Cliftonz It will be even shorter
email -> dns -> inbound-email service-> queue -> email parse (on worker)
After we parse the email we respond user's webhook

@Cliftonz
Copy link
Contributor

Cliftonz commented Jan 9, 2024

@djabarovgeorge Then I do not think we need to make any changes as the general worker service can handle the load of at most 10 jobs per month.

@Cliftonz
Copy link
Contributor

@djabarovgeorge There are a few conflicts but I would like to get this merged in soon

…ser-to-worker

# Conflicts:
#	apps/worker/src/app/workflow/workflow.module.ts
Copy link

netlify bot commented Jan 22, 2024

Deploy Preview for dev-web-novu failed.

Name Link
🔨 Latest commit 5d8b78b
🔍 Latest deploy log https://app.netlify.com/sites/dev-web-novu/deploys/65b8d0dd0a7b670008d1e457

@djabarovgeorge
Copy link
Contributor Author

@djabarovgeorge There are a few conflicts but I would like to get this merged in soon

Tend to agree here 💪

@Cliftonz Cliftonz merged commit 06f9154 into next Jan 30, 2024
25 of 30 checks passed
@Cliftonz Cliftonz deleted the move-inbound-mail-parser-to-worker branch January 30, 2024 15:01
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.

None yet

4 participants