🎁 Pretty print cloud events #25
Conversation
cardil
force-pushed
the
feature/pretty-print
branch
from
772bd73
to
5b93b0e
Compare
cardil
force-pushed
the
feature/pretty-print
branch
from
5b93b0e
to
cc91f5f
Compare
There was a problem hiding this comment.
Hey @cardil , I'm still playing with it but overall looks good to me.
I left some comments about putting , in the output vs. using :
Should the whole output on the console be json ? Or just the body might be json? (ok, I think just the body in the event of application/json contenttype)
This is my early feedback, will let you know when I'm done reviewing it.
| * @returns {string} - the buffer | ||
| */ | ||
| function printAttr(attr) { | ||
| let buf = 'Context Attributes,\n' |
There was a problem hiding this comment.
Should there be : after Context Attributes?
Running the tests, it gives me this:
Received event:
☁️ cloudevents.Event
Validation: valid
Context Attributes,
specversion: 1.0
type: com.redhat.openshift.knative.showcase.Hello
source: //localhost/dev
id: 780ed118-9509-4d7b-a612-97f278eb2b55
time: 2022-02-24T02:15:27.522Z
datacontenttype: application/json
Data,
{
"greeting": "Welcome",
"who": "Developer31",
"number": 64
}
There was a problem hiding this comment.
@mgencur fwiw, this is the format that the golang cloudevents SDK prints events. I'm not saying one way or the other how it should be printed here other than to note that there is precedent for this format.
There was a problem hiding this comment.
I guess I should have read all of the comments, including @cardil's first. 🤣
There was a problem hiding this comment.
For the original viewer app, the output was just to print the event using the String() method. https://github.com/cloudevents/sdk-go/blob/ff0a142752d3253c3b974246c07f450421714f23/v2/event/event.go#L70-L97
There was a problem hiding this comment.
Thanks, ok good to know. I guess this is alright. Not saying I like it but we can use it as it's used elsewhere.
| let buf = '' | ||
| const exts = extensions(ce, attr) | ||
| if (Object.keys(exts).length > 0) { | ||
| buf += `Extensions,\n` |
There was a problem hiding this comment.
Should there be : after Extensions?
See my previous comment about the output.
| var buf = new StringBuilder(); | ||
| var data = ce.getData(); | ||
| if (data != null) { | ||
| buf.append("Data,\n"); |
There was a problem hiding this comment.
Should there be : after Data instead of comma?
| function printData(ce) { | ||
| let buf = '' | ||
| if (ce.data) { | ||
| buf += `Data,\n` |
| var buf = new StringBuilder(); | ||
| var exts = extensions(ce); | ||
| if (!exts.isEmpty()) { | ||
| buf.append("Extensions,\n"); |
| private CharSequence printAttr(Map<String, Serializable> attr) { | ||
| var buf = new StringBuilder(); | ||
| if (!attr.isEmpty()) { | ||
| buf.append("Context Attributes,\n"); |
|
The idea behind this exact format is to mimic the eventing event-display app: https://github.com/knative/eventing/blob/main/cmd/event_display/main.go#L41 The event display app is currently used in our docs, so I thought this would be the easier path to transition for our customers. |
|
Apparently, there's no easy way to build the target images locally. I guess we can merge it and I will check the image once the github action builds it. |
cardil
force-pushed
the
feature/pretty-print
branch
from
2823d82
to
cc91f5f
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cardil, mgencur 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 |
Changes
Fixes #15
Follow up to #24