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: REST - create rest/builder.nim and simplify factory/app.nim #2610

Closed
wants to merge 1 commit into from

Conversation

Ivansete-status
Copy link
Collaborator

Description

Extract some REST logic from app.nim, aiming for simplify app.nim a little.
I will submit a couple of similar PRs simplifying app.nim.

Issue

With that simplification, this PR indirectly contributes to the following: migrate DiscV5 and DNS Discovery from app.nim to waku_node.nim #2452

@Ivansete-status Ivansete-status changed the title chore: REST - create rest/builder.nim and simplify factory/app.nim refactor: REST - create rest/builder.nim and simplify factory/app.nim Apr 22, 2024
Copy link

github-actions bot commented Apr 22, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2610-rln-v2-true

Built from 60568ac

@Ivansete-status Ivansete-status marked this pull request as ready for review April 22, 2024 12:11
Copy link
Contributor

@SionoiS SionoiS left a comment

Choose a reason for hiding this comment

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

LGTM

I like this a lot!

Copy link
Contributor

@NagyZoltanPeter NagyZoltanPeter left a comment

Choose a reason for hiding this comment

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

I think we are crossing our works. Can you please take a look at mine. I will draft it now. I'm just adding a final json serialization for a bit extended /health endpoint.
To enable separation of initialization health from other endpoints and start rest server earlier, I needed to rework to order of rest initialization.
Still, I agree to simplify waku node initialization order due I found its overly complicated.

Copy link
Contributor

@gabrielmer gabrielmer left a comment

Choose a reason for hiding this comment

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

Looks amazing! Thanks so much! Love this change 😍

Just added a tiny question

waku/factory/app.nim Show resolved Hide resolved
@gabrielmer gabrielmer self-requested a review April 22, 2024 15:08
Copy link
Contributor

@gabrielmer gabrielmer left a comment

Choose a reason for hiding this comment

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

Thanks so much!

@Ivansete-status
Copy link
Collaborator Author

Closing because has been solved in #2623

@Ivansete-status Ivansete-status deleted the refactor-discovery branch May 26, 2024 20:38
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.

4 participants