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

OpenAPI for application management #184

Open
wants to merge 34 commits into
base: main
Choose a base branch
from
Open

Conversation

tuler
Copy link
Member

@tuler tuler commented Dec 4, 2023

Defines an openapi for application management of a (future) multi-application node.
It defines 4 endpoints: listing, adding, querying and removing.

Listing is paginated and has filtering based on status.

I used application instead of dapp. We're favoring application in several locations including sunodo codebase and rollups-explorer codebase, because there are several tools that generate names based on that, and the term "DApp", or "dApp" does not behave well with those tools, often generating oddities like d-app, or d_app.

@tuler tuler requested a review from gligneul December 4, 2023 18:49
@gligneul gligneul added the no changelog PRs that don't require changes in changelog label Dec 4, 2023
@gligneul gligneul added the #feat:multi-dapp Feature: multi-dapp node label Dec 4, 2023
@tuler tuler force-pushed the feature/management-api branch from 36df0ba to cbbd342 Compare December 4, 2023 21:25
Copy link
Contributor

@gligneul gligneul left a comment

Choose a reason for hiding this comment

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

Looking good

api/openapi.yaml Outdated Show resolved Hide resolved
api/openapi.yaml Outdated Show resolved Hide resolved
api/openapi.yaml Outdated Show resolved Hide resolved
api/openapi.yaml Outdated Show resolved Hide resolved
api/openapi.yaml Outdated Show resolved Hide resolved
api/openapi.yaml Outdated Show resolved Hide resolved
api/openapi.yaml Outdated Show resolved Hide resolved
api/openapi.yaml Outdated Show resolved Hide resolved
@gligneul gligneul added this to the 1.3.0 milestone Dec 4, 2023
@tuler tuler requested a review from gligneul December 5, 2023 02:19
@gligneul gligneul requested a review from endersonmaia December 5, 2023 13:13
@endersonmaia
Copy link
Contributor

I found no reference to the chain the node is running?

Is it expected to a multi-dapp node to work on multiple chains, of for a single chain?

Anyway, I think some API endpoint (or response header) should expose details for the configuration of the node, the chainID being one of these details.

GET /v1/status
{ "chainID":  "123", "epochDuration": "68000", "applicationCount": 10, ...}

@gligneul gligneul requested review from torives and removed request for marcelstanley December 5, 2023 15:01
@tuler
Copy link
Member Author

tuler commented Dec 5, 2023

Is it expected to a multi-dapp node to work on multiple chains, of for a single chain?

No. It's single chain.

Anyway, I think some API endpoint (or response header) should expose details for the configuration of the node, the chainID being one of these details.

The process running the API will receive the same env variables as a node would receive.
Aiming for simplicity, I don't think we need to expose this to an endpoint.

@tuler
Copy link
Member Author

tuler commented Dec 5, 2023

"applicationCount": 10

Application count is part of the API in the response of the GET /applications

api/openapi.yaml Outdated Show resolved Hide resolved
api/openapi.yaml Outdated Show resolved Hide resolved
api/openapi.yaml Outdated Show resolved Hide resolved
api/management.openapi.yaml Outdated Show resolved Hide resolved
api/openapi.yaml Outdated Show resolved Hide resolved
api/management.openapi.yaml Outdated Show resolved Hide resolved
gligneul
gligneul previously approved these changes Dec 6, 2023
Copy link
Contributor

@gligneul gligneul left a comment

Choose a reason for hiding this comment

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

Looks good!

@tuler
Copy link
Member Author

tuler commented Dec 6, 2023

Rebased, squashed, renamed file.

gligneul
gligneul previously approved these changes Dec 6, 2023
@tuler
Copy link
Member Author

tuler commented Dec 6, 2023

Added 204 to DELETE, in case it can be satisfied immediately.
I'm working on the implementation, and it looks like the case will be that we will send a SIGTERM to gracefully shutdown a node, and wait for it to die with some timeout. If it succeeds we can return 204, if it times out we can return a 202. Does it make sense?

@tuler tuler force-pushed the feature/management-api branch 2 times, most recently from 7ae3ced to 12db9e3 Compare December 6, 2023 14:40
@gligneul
Copy link
Contributor

gligneul commented Dec 6, 2023

I'm working on the implementation, and it looks like the case will be that we will send a SIGTERM to gracefully shutdown a node, and wait for it to die with some timeout. If it succeeds we can return 204, if it times out we can return a 202. Does it make sense?

So, will it be a sync request? Do you plan to remove the stopping and stopped status?

@tuler
Copy link
Member Author

tuler commented Dec 6, 2023

So, will it be a sync request? Do you plan to remove the stopping and stopped status?

It will be a sync request. If times out the client would have to query the GET until it returns stopped, or if we remove the stopped until it returns 404.

@gligneul
Copy link
Contributor

gligneul commented Dec 6, 2023

It will be a sync request. If times out the client would have to query the GET until it returns stopped, or if we remove the stopped until it returns 404.

I see. I don't have strong opinions about the stopped, you can leave it if you want.

marcelstanley and others added 29 commits July 8, 2024 20:04
Added general '-A clippy::mixed_attributes_style' to CI clippy execution to avoid linter warning on the automatically generated file 'iconsensus.rs'
Applied suffixes ENABLED to boolean parameters to match the node
internal state and set their appropriate default values.
- Adds linux.bin and rootfs.ext2 download to the emulator stage.
- Adds a emulator-devel stage that install libcmt and xgenext2fs.
- Configures the CI image to use CGO.
- Refactors the Dockerfile to improve readability.
@vfusco vfusco force-pushed the feature/management-api branch from 2a84f5d to 0f9100b Compare July 12, 2024 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
#feat:multi-dapp Feature: multi-dapp node no changelog PRs that don't require changes in changelog
Projects
Status: 🏗 In progress
Development

Successfully merging this pull request may close these issues.

7 participants