-
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
Implement workflow show
#458
Conversation
// TODO: Do we need to do the "time elapsed" thing? Also needs some redoing to print | ||
// result at the end, since final event is not accessible from here. |
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.
Looking for any thoughts on this
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.
ON workflow execute
I think, there was this "Progress" with a counting number of seconds which I removed, and if that's what this was here, I think we can remove it here too.
if err := iter.print(cctx.Printer); err != nil { | ||
return fmt.Errorf("displaying history failed: %w", err) | ||
} | ||
} 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.
It is important to note that IIUC workflow show
is the primary way people dump workflow history for use in replayers. So for JSON, we need to dump exact history format (i.e. create the history.History
proto object with its events and send that to PrintStructured
, which is close to what's here but has to be the full thing)
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 is exactly how the old CLI did it FYI (for the outer wrapper with 'events' field, but of course it used real HistoryEvent protos which is more important). But, making it be a literal history proto makes more sense anyway.
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.
But, making it be a literal history proto makes more sense anyway.
They used that in printReplayableHistory
which makes sense (it's simple code just for JSON), but for the non-JSON, yeah the lazy stuff for text is a bit more difficult. Feel free to steal anything from workflow execute
which does similar.
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
@@ -438,8 +438,8 @@ Use the options listed below to change the command's behavior. | |||
|
|||
#### Options | |||
|
|||
* `--reset-points` (bool) - Only show auto-reset points. |
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.
Looks like old CLI accepted this but never used it, 👍 removing
What was changed
Implement the
workflow show
commandWhy?
Part of cli rewrite
Checklist
Closes
How was this tested: