-
Notifications
You must be signed in to change notification settings - Fork 600
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
change content-type of performance tests to application/json #2010
Conversation
/assign @grantr |
/retest |
@capri-xiyue why this change? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to change the content type to application/json
, then you must quote the body string to make the json valid. An unquoted string is not a valid application/json
body
/retest |
FYI #2009. Currently cloudevents sdk doesn't support Content-type "text/plain" |
@slinkydeveloper, this is how the performance test sends cloudevents
}` |
/assign @grantr |
The reason I avoided json is to avoid the additional parsing step the receiver does when it receives the message. Maybe can we use a binary format like |
This one:
Should be:
|
@slinkydeveloper Currently cloudevents sdk-go only supports only supports type as follows: |
The request body should be valid given the content-type, so we should add the quotes. When cloudevents/sdk-go#232 is fixed we can change the content-type back. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: capri-xiyue, grantr The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Fixes #2009
Proposed Changes
Release Note