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

Added datacontentencoding #387

Merged
merged 3 commits into from Apr 4, 2019
Merged

Added datacontentencoding #387

merged 3 commits into from Apr 4, 2019

Conversation

clemensv
Copy link
Contributor

@clemensv clemensv commented Feb 15, 2019

Implements content encoding as raised in issue #371

Signed-off-by: Clemens Vasters <clemensv@microsoft.com>
@duglin
Copy link
Collaborator

duglin commented Feb 18, 2019

ping @alanconway @n3wscott
Any thoughts on this?

@alanconway
Copy link
Contributor

The datacontentencoding changes are all good by me.

On a side note: I'm surprised that critical attribute names are still changing, e.g. "contenttype -> datacontenttype". Also since the spec has adopted the cantdecideonwordseparatorssoletshavenone convention, it seems like a not-so-great idea to be adding more words to multi-word names. However, that's another story.

@duglin
Copy link
Collaborator

duglin commented Feb 21, 2019

Would an example help clarify the need for this to a newbie? Could be done in a follow-on PR if so.

attribute for when the `data` field MUST be encoded as a string,
like with structured transport binding modes using the JSON event
format, but the `datacontenttype` indicates a non-string media
type. When the `data` field's effective data type is not `String`,
Copy link
Member

Choose a reason for hiding this comment

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

When the data field's effective data type is not String,
this attribute MUST NOT be set and MUST be ignored when set.

why not? I want to zip my http binary data payload...

Copy link
Member

Choose a reason for hiding this comment

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

Or I want to base64 encode my binary http payload because I am a weirdo...

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps not, looking at RFC 2045 I do not see anything I would want to use besides base64. And normal http transports can give me gzip...

/lgtm

@alanconway
Copy link
Contributor

alanconway commented Feb 21, 2019 via email

the `data` attribute towards the application is the base64-decoded
binary array.

All other RFC2045 schemes are undefined for CloudEvents.
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a "Constraints" section, like the other attributes?

I think at the moment, "Constraints" is "Must be the string 'Base64'". If so, would it make more sense to standardize on assuming 'Base64' encoding and excluding the other options?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@clemensv ^^^

@alanconway
Copy link
Contributor

alanconway commented Mar 7, 2019 via email

Signed-off-by: Clemens Vasters <clemensv@microsoft.com>
@clemensv
Copy link
Contributor Author

@alanconway we chose to rename contenttype to datacontenttype because people were confused about what that relates to, and now this also needs to have data in front of it, because it relates to the same attribute.

@duglin
Copy link
Collaborator

duglin commented Mar 15, 2019

All - This one has been updated - please review and comment

@rperelma
Copy link
Contributor

Might be good to add an example

@JemDay
Copy link
Contributor

JemDay commented Mar 27, 2019

+1 for some examples - but maybe there should be a separate PR to add a few examples based on recent changes.

Generally LGTM but deferring final comment until merge conflicts are resolved.

@duglin
Copy link
Collaborator

duglin commented Apr 4, 2019

Approved on the 4/4 call

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.

None yet

7 participants