-
Notifications
You must be signed in to change notification settings - Fork 42
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
Supported flattened event detail view on workflow show/execute #615
Conversation
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.
Yay, this is so much better
} else { | ||
s.Contains(output, "WorkflowExecutionSignaled") | ||
} |
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.
This would show up in both output kinds, right? Non-detailed on the line, and detailed in the section header.
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.
Yes, but before this the test tested nothing about non-detailed event output, so I just thought I'd add it in an else
for code clarity reasons (I can do better assertions on detailed). But I can move it out of else.
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.
Yeah I'd just assert it either way
sort.Slice(fields, func(i, j int) bool { return fields[i].field < fields[j].field }) | ||
return fields, nil |
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.
For arrays, will sorting do alphabetical sort as opposed to numerical "7" > "10"
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.
It might, good catch! I will fix this and add a test for it.
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.
This looks great, so much cleaner than before
Once stabilized, maybe add it to the new help to promote it? |
What was changed
--event-details
fromworkflow show
andworkflow execute
that, in text mode, blindly dumped full event in table column in an unreadable way--detailed
toworkflow show
andworkflow execute
that, for text output, dumps events as sections--follow
forworkflow show
) still supportedBefore with something like
temporal workflow show -w my-wf --event-details
:(note how bad this would be wrapping in a terminal)
After with something like
temporal workflow show -w my-wf --detailed
:Note, the table form still exists for non
--detailed
. Also note that we can continually tweak this post-merge, don't need to block merge for little pieces.Checklist
workflow show
Details more readable #546