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

Attachment support for pub/sub #318

Merged
merged 21 commits into from
Jan 18, 2024
Merged

Attachment support for pub/sub #318

merged 21 commits into from
Jan 18, 2024

Conversation

p-avital
Copy link
Contributor

Since we need pub/sub attachments earlier, and they're easier to implement, we merge them before enabling attachments in query/replies.

This PR also paves the way for query/replies attachment support, but their API still needs some work to ensure ownership stays clear.

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

Cppcheck (reported by Codacy) found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

@p-avital
Copy link
Contributor Author

@jean-roland can I have a review? :D

@p-avital p-avital requested a review from Mallets January 16, 2024 15:19
examples/unix/c11/z_put.c Outdated Show resolved Hide resolved
examples/unix/c11/z_sub.c Outdated Show resolved Hide resolved
@jean-roland
Copy link
Contributor

jean-roland commented Jan 18, 2024

LGTM but since we are in a modularization effort in Zenoh-Pico, I believe there should be a Z_FEATURE_ATTACHMENT token that toggles the API and transcoding logic

@jean-roland
Copy link
Contributor

jean-roland commented Jan 18, 2024

Now this is only the first phase of attachments so that'll do, but for the final one we'd need a CI test checking that both configuration works and an attachment update on z_msgcodec_test and z_client_test.

** Approved **

src/api/api.c Outdated Show resolved Hide resolved
src/api/api.c Outdated Show resolved Hide resolved
@Mallets Mallets merged commit 5e87691 into main Jan 18, 2024
94 checks passed
@Mallets Mallets deleted the attachment branch January 18, 2024 13:30
@jean-roland jean-roland mentioned this pull request Jan 19, 2024
@gabrik gabrik added the new feature Something new is needed label Apr 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature Something new is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants