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

Add Remote UDP extension #515 #538

Merged
merged 4 commits into from
Jul 1, 2024
Merged

Add Remote UDP extension #515 #538

merged 4 commits into from
Jul 1, 2024

Conversation

m-mohr
Copy link
Member

@m-mohr m-mohr commented Jun 20, 2024

Implements an extension to enable loading user-defined processes that are hosted externally through the process namespace into process graphs.

Closes #515

@m-mohr m-mohr added processes Process definitions and descriptions process graphs labels Jun 20, 2024
@m-mohr m-mohr added this to the 1.3.0 milestone Jun 20, 2024
extensions/remote-udp/README.md Outdated Show resolved Hide resolved
extensions/remote-udp/README.md Outdated Show resolved Hide resolved
extensions/remote-udp/README.md Outdated Show resolved Hide resolved
extensions/remote-udp/README.md Outdated Show resolved Hide resolved
extensions/remote-udp/README.md Show resolved Hide resolved
@m-mohr m-mohr requested a review from soxofaan June 27, 2024 21:00
extensions/remote-udp/README.md Outdated Show resolved Hide resolved
extensions/remote-udp/README.md Outdated Show resolved Hide resolved
extensions/remote-udp/README.md Outdated Show resolved Hide resolved
extensions/remote-udp/README.md Outdated Show resolved Hide resolved
Co-authored-by: Stefaan Lippens <soxofaan@users.noreply.github.com>
@m-mohr
Copy link
Member Author

m-mohr commented Jun 28, 2024

@soxofaan Updated in b3c7cef - Please check again.

Copy link
Member

@soxofaan soxofaan left a comment

Choose a reason for hiding this comment

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

some final notes, but I think the core is fine to merge as-is

extensions/remote-udp/README.md Outdated Show resolved Hide resolved

### Error Handling

The following error SHOULD be reported if the namespace can't be resolved:
Copy link
Member

Choose a reason for hiding this comment

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

What if the URL can be resolved, but the document does not fit? e.g. it's not JSON, it does not follow expected schema, it doesn't contain a process_graph property, ...?

Shouldn't this also be a ProcessNamespaceInvalid?
Or would that be for standard errors like ProcessInvalid, ProcessGraphMissing? Note however that these don't have placeholders like for a reason, so that could lead to poor error messages

Copy link
Member Author

@m-mohr m-mohr Jul 1, 2024

Choose a reason for hiding this comment

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

Yeah, I guess that should fallback to the standard errors. Generally, back-ends can also define own error messages and codes. If specific common cases occur that are not covered and need a reason, we can fine-tune this later. Please give feedback :-)

@m-mohr m-mohr merged commit caef6e4 into draft Jul 1, 2024
2 checks passed
@m-mohr m-mohr deleted the remote-udp branch July 1, 2024 13:06
@m-mohr m-mohr linked an issue Jul 1, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
process graphs processes Process definitions and descriptions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow URLs as process namespace
2 participants