Skip to content

Adding consumer for uSubscription service compatible with up-spec alpha4#305

Merged
PLeVasseur merged 10 commits intoeclipse-uprotocol:mainfrom
lukas-he:feature/add_usubscription_client
Mar 3, 2025
Merged

Adding consumer for uSubscription service compatible with up-spec alpha4#305
PLeVasseur merged 10 commits intoeclipse-uprotocol:mainfrom
lukas-he:feature/add_usubscription_client

Conversation

@lukas-he
Copy link
Contributor

@lukas-he lukas-he commented Feb 13, 2025

This PR reopens the PR #292. In addition, the Consumer is upgraded to the release alpha4 of the spec. The upgrade to alpha4 came with changes in the usubscription.proto .

@lukas-he lukas-he force-pushed the feature/add_usubscription_client branch from 2dfc08b to c1dcf6e Compare February 18, 2025 11:36
@github-actions
Copy link

github-actions bot commented Mar 3, 2025

Code coverage report is ready! 📈

@PLeVasseur
Copy link
Contributor

PLeVasseur commented Mar 3, 2025

@lukas-he -- ran the workflows, looks like a lint issue came up. Didn't investigate from my end.

Ah, due to this? #307

@lukas-he
Copy link
Contributor Author

lukas-he commented Mar 3, 2025

@lukas-he -- ran the workflows, looks like a lint issue came up. Didn't investigate from my end.

Ah, due to this? #307

No, the errors from #307 are also fixed here (to look if it keeps failing despite fixing it). #307 resolved the linting errors. Here it is failing due to linting warnings. There are lots of warnings, so would it make sense to resolve all clang-tidy warnings in a separate issue and approve despite them?

@PLeVasseur
Copy link
Contributor

No, the errors from #307 are also fixed here (to look if it keeps failing despite fixing it). #307 resolved the linting errors.

Okay, got it.

Here it is failing due to linting warnings. There are lots of warnings, so would it make sense to resolve all clang-tidy warnings in a separate issue and approve despite them?

Sure, would you like to open an issue for resolving the warnings?

Also -- I think this needs to be rebased, then we can spin the CI again and go for a merge.

@github-actions
Copy link

github-actions bot commented Mar 3, 2025

Code coverage report is ready! 📈

Copy link
Contributor

@PLeVasseur PLeVasseur left a comment

Choose a reason for hiding this comment

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

LGTM. Seems to be a cleanly opened version of #292 which went through code review already.

@PLeVasseur PLeVasseur merged commit 7ba0e64 into eclipse-uprotocol:main Mar 3, 2025
11 of 12 checks passed
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.

3 participants