-
Notifications
You must be signed in to change notification settings - Fork 13
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 dns flows and workers #349
Conversation
@Genne23v it would have been better to separate the new functionality you've added from the refactoring. Is there any way to split this PR into smaller PRs? If not, I'll review it as is. But this isn't ideal. |
I can separate. I will push an update soon. |
9d1ad06
to
6cdfbac
Compare
@humphd I have removed new functionalities although they are very small. I would appreciate your review. |
Can you rebase and fix CI before I review? |
Let me connect with frontend that is just merged. |
8ca7ec2
to
09cd66e
Compare
@humphd @dadolhay I connected DNS module with frontend. It's functional, but it needs more backend error validation and display the error somewhere in frontend. Let me know if anything needs to be added. |
@dadolhay @humphd I updated the PR. I would appreciate another round of review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made an initial set of comments. Looking good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two small things. This is looking so much better, great work.
Only took 2 days, not bad! |
Close #315
Close #333
How to test
Currently integration is only set up in
/dev
route. You don't need any environment setup to test.I'm not sure how DNS handling should be initiated after DB update (same frontend call or background job).
@dadolhay This update requires DB table ID in
addDnsRequest
call in your function. Please let me know if you have any concern with this approach.