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.

Sign up for GitHub

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

Jump to bottom

Remove uPayload #145

Merged
merged 7 commits into from
May 16, 2024
Merged

Remove uPayload #145

merged 7 commits into from
May 16, 2024

Conversation

stevenhartley
Copy link
Contributor

The following change removes the unused UPayload protobuf object and places the format in UAttributes.

#144

Remove uPayload
8839930 
The following change removes the unused UPayload protobuf object and places the format in UAttributes.

#144
@gregmedd
Copy link
Contributor

This looks like an otherwise minor change to the structure of a message that has fairly large implications for the APIs we are building. For example, this will require non-trivial changes to the up-cpp API update we just reviewed and approved yesterday as it completely changes the interaction model for message data.

It also removes the possibility of extending the UPayload type in the future should it be necessary to support more advanced features, although I can see that the intent is that those would be represented as new data types so maybe extensibility doesn't matter so much.

Are we really sure it is worth making this change? If nothing else, it will further delay the up-cpp API updates while we make this change.

PLeVasseur
PLeVasseur reviewed May 15, 2024
View reviewed changes
basics/uattributes.adoc Outdated Show resolved Hide resolved
@PLeVasseur
Update basics/uattributes.adoc
880286f 
Co-authored-by: Pete LeVasseur <peter.levasseur@gm.com>
@stevenhartley stevenhartley marked this pull request as draft May 15, 2024 20:14
switch back to UPayloadFormat to avoid unnecessary changes and since …
887d889 
…we refer to payload everywhere in the specs
@stevenhartley stevenhartley marked this pull request as ready for review May 16, 2024 00:13
sophokles73
sophokles73 reviewed May 16, 2024
View reviewed changes
Copy link
Contributor

@sophokles73 sophokles73 left a comment

Choose a reason for hiding this comment

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

FMPOV the up-l1/cloudevents-adoc file also needs to be adapted accordingly ...

basics/upayloadformat.adoc Outdated Show resolved Hide resolved
up-core-api/uprotocol/uattributes.proto Outdated Show resolved Hide resolved
basics/upayloadformat.adoc Outdated Show resolved Hide resolved
basics/upayloadformat.adoc Outdated Show resolved Hide resolved
up-core-api/uprotocol/umessage.proto Show resolved Hide resolved
PLeVasseur
PLeVasseur approved these changes May 16, 2024
View reviewed changes
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

sophokles73
sophokles73 approved these changes May 16, 2024
View reviewed changes
Copy link
Contributor

@sophokles73 sophokles73 left a comment

Choose a reason for hiding this comment

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

LGTM

@stevenhartley stevenhartley merged commit 18fadea into main May 16, 2024
2 checks passed
@stevenhartley stevenhartley deleted the upayload branch May 16, 2024 16:52
@AnotherDaniel AnotherDaniel mentioned this pull request May 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PLeVasseur PLeVasseur PLeVasseur approved these changes

@sophokles73 sophokles73 sophokles73 approved these changes

@gregmedd gregmedd Awaiting requested review from gregmedd

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@stevenhartley @gregmedd @sophokles73 @PLeVasseur