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

Modified content-type to abide by attribute naming conventions for cloudevents #232

Merged
merged 5 commits into from
May 26, 2024

Conversation

vivjd
Copy link
Contributor

@vivjd vivjd commented Jan 6, 2024

Fixes #211

Changes

The content-type attribute will violate the attribute naming rules, so the attribute has been modified to datacontenttype.

One line description for the changelog

Converted content-type to datacontenttype for cloudevents.

  • Tests pass

@xSAVIKx
Copy link
Member

xSAVIKx commented Jan 6, 2024

Hey @vivjd, thx for opening the PR.

Unfortunately your change is not really what the desired behavior should be. I've mentioned the desired one here: #211 (comment)

So if you're up for changing this to comply with the desired behavior I'd be glad to review the PR.

@vivjd
Copy link
Contributor Author

vivjd commented Jan 20, 2024

Hi @xSAVIKx! Thank you for you feedback. I've reread the original issue and the desired behavior, and changed it accordingly in my PR. If you could review the PR that would be great! Thank you for your time!

Copy link
Member

@xSAVIKx xSAVIKx left a comment

Choose a reason for hiding this comment

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

@vivjd overall LGTM. Would you be so kind to add a test case for this change in kafka-related test suite?

@vivjd
Copy link
Contributor Author

vivjd commented Mar 31, 2024

Hi @xSAVIKx, I changed the test cases to match the change. Could you please kindly review this again? Thank you in advance!

@vivjd vivjd requested a review from xSAVIKx March 31, 2024 16:07
@Cali0707
Copy link

Cali0707 commented Apr 6, 2024

Hey @xSAVIKx @duglin would one of you be able to approve the workflows on this PR?

@xSAVIKx
Copy link
Member

xSAVIKx commented Apr 6, 2024

Sorry, my bad. I was pretty sure I did that when I was reviewing the PR

@xSAVIKx xSAVIKx merged commit 16441d7 into cloudevents:main May 26, 2024
16 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.

cloudevents.kafka.to_binary checks for a content-type attribute, which would not be a valid attribute name
3 participants