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.

Sign up for GitHub

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

Jump to bottom

Add current state to workflow states table and format duration time #9191

Merged
merged 1 commit into from
May 24, 2024
Merged

Add current state to workflow states table and format duration time #9191

merged 1 commit into from
May 24, 2024

Conversation

GilbertCherrie
Copy link
Member

@GilbertCherrie GilbertCherrie commented May 16, 2024 •
edited

Fixes: #9189

Adds the current state to the workflow states table. Also, formats the duration time to be displays as a string in days hours minutes seconds format instead of just seconds.

Before:
Screenshot 2024-05-17 at 9 43 27 AM

After:
Screenshot 2024-05-23 at 4 53 58 PM

@GilbertCherrie GilbertCherrie requested a review from a team as a code owner May 16, 2024 16:55
@GilbertCherrie GilbertCherrie mentioned this pull request May 16, 2024
Closed
jeffibm
jeffibm reviewed May 16, 2024
View reviewed changes
Copy link
Member

@jeffibm jeffibm left a comment

Choose a reason for hiding this comment

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

Also, if possible, could you please post a before/after screenshot of the page from where this change is coming in from?

app/javascript/components/request-workflow-status/data.js Outdated Show resolved Hide resolved
@Fryguy
Copy link
Member

Fryguy commented May 16, 2024

I'm concerned this will appear like the step is done. Wonder if we can have a leading icon that represent the current state of the state

@GilbertCherrie
Copy link
Member Author

GilbertCherrie commented May 16, 2024 •
edited

@Fryguy @agrare Here is a quick example:

Screenshot 2024-05-16 at 6 01 08 PM

Wanted to get your thoughts on this new row and the icon I chose. For the duration I used Adam's suggestion of entered time - current time.

@agrare
Copy link
Member

agrare commented May 16, 2024

Looks good @GilbertCherrie I wonder if it's possible to format as N days M hours T seconds bit that's not critical

@GilbertCherrie GilbertCherrie force-pushed the fix_workflow_states_table branch 4 times, most recently from b133310 to 08934ea Compare May 17, 2024 13:31
@GilbertCherrie GilbertCherrie changed the title Add current state to workflow states table Add current state to workflow states table and format duration time May 17, 2024
@GilbertCherrie
Copy link
Member Author

@agrare I made all the requested changes if you can review this again

@Fryguy
Copy link
Member

Fryguy commented May 17, 2024

Looks good @GilbertCherrie I wonder if it's possible to format as N days M hours T seconds bit that's not critical

More specifically the formatting should match whatever the other columns show.

@Fryguy
Copy link
Member

Fryguy commented May 17, 2024

@Fryguy @agrare Here is a quick example:
Screenshot 2024-05-16 at 6 01 08 PM

Wanted to get your thoughts on this new row and the icon I chose. For the duration I used Adam's suggestion of entered time - current time.

That's a good choice... I was also thinking something like possibly

  • for running (with a yellow css color)
    • fa-spinner (specifically with a css rotation animation)
    • fa-ellipsis
    • fa-circle-play
  • for completed (with a green css color - similar to the flash icon)
    • fa-circle-check
  • for failed (with a red css color - similar to the flash icon)
    • fa-circle-xmark

@GilbertCherrie GilbertCherrie force-pushed the fix_workflow_states_table branch 4 times, most recently from 82aca2e to 58c913b Compare May 21, 2024 15:33
@GilbertCherrie
Copy link
Member Author

GilbertCherrie commented May 21, 2024 •
edited

@agrare @Fryguy @jeffibm I fixed the icons if you can review again when you have time. Also, @Fryguy I changed the data table to be able to use carbon icons if needed so now this table uses carbon icons, I still used the same symbols that you wanted but just the carbon equivalents.

Fryguy
Fryguy reviewed May 21, 2024
View reviewed changes
app/javascript/components/request-workflow-status/data.js
// Converts the duration time in ms and returns a string in format: w days x hours y minutes z seconds
// duration: time in ms
const convertDuration = (duration) => {
const durationString = moment.duration(duration, 'milliseconds').toISOString().split('PT')[1];
Copy link
Member

Choose a reason for hiding this comment

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

I'm very surprised this doesn't "throw away" the days, months, years component of the time

Fryguy
Fryguy reviewed May 21, 2024
View reviewed changes
app/javascript/components/request-workflow-status/data.js
@@ -59,6 +89,20 @@ export const workflowStatusData = (response) => {
return undefined;
}
const rows = response.context ? rowData(response.context) : [];
if (response.context && response.context.State) {
Copy link
Member

Choose a reason for hiding this comment

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

@agrare I think this code depends on nil-ing out the state when the flow is completed. Is that in yet?

Copy link
Member

@agrare agrare May 21, 2024 •
edited

Choose a reason for hiding this comment

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

No it isn't, started it but ran into some issues because we use context.output after a workflow has completed so need to tweak some stuff to be able to do this in floe.

Copy link
Member

Choose a reason for hiding this comment

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

@GilbertCherrie Since the "final" state might appear twice, I'm concerned this will double up the presentation of it. @agrare Thoughts on how we can avoid presenting the final state twice?

Copy link
Member Author

Choose a reason for hiding this comment

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

@Fryguy @agrare What if I add an ID check where if the current state id is found in an array of all the past ids it won't render the current state? The array might be a little large based on how many past states there is but its one possible solution if we want to handle the fix in the front end.

Copy link
Member

Choose a reason for hiding this comment

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

@agrare If we do a state retry or a state gets re-run in a loop, does each iteration get it's own GUID? If so, then yeah, an ID check might work.

@GilbertCherrie You could also just check the very last state in the StatesHistory and if it's the same as the "current" state then don't show it.

Copy link
Member

@agrare agrare May 24, 2024 •
edited

Choose a reason for hiding this comment

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

If we do a state retry or a state gets re-run in a loop, does each iteration get it's own GUID? If so, then yeah, an ID check might work.

Here is an example of the same state retried twice from floe specs:

"StateHistory"=>
    [{"Name"=>"State",
      "Input"=>{"foo"=>{"bar"=>"baz"}, "bar"=>{"baz"=>"foo"}},
      "Guid"=>"63837184-0795-41df-8d23-da034b8b2850",
      "EnteredTime"=>"2024-05-24T12:49:47Z",
      "RunnerContext"=>{"container_ref"=>"container-d"},
      "Output"=>{"Error"=>"States.Timeout"},
      "NextState"=>nil,
      "FinishedTime"=>"2024-05-24T12:49:47Z",
      "Duration"=>0.423689375,
      "RetryCount"=>2,
      "Retrier"=>["States.Timeout"]},
     {"Name"=>"State",
      "Input"=>{"foo"=>{"bar"=>"baz"}, "bar"=>{"baz"=>"foo"}},
      "Guid"=>"63837184-0795-41df-8d23-da034b8b2850",
      "EnteredTime"=>"2024-05-24T12:49:47Z",
      "RunnerContext"=>{"container_ref"=>"container-d"},
      "Output"=>{"Error"=>"States.Timeout"},
      "NextState"=>nil,
      "FinishedTime"=>"2024-05-24T12:49:47Z",
      "Duration"=>0.423689375,
      "RetryCount"=>2,
      "Retrier"=>["States.Timeout"]}]

The State gets a Guid when it is started and a state that is retried multiple times is still the same state so it has the same Guid.

Copy link
Member

Choose a reason for hiding this comment

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

@agrare One more question. When the entire flow is done, does the "current" state have a "finished time"? If so, that might be the simplest check.

Copy link
Member

Choose a reason for hiding this comment

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

Yes,

   "State"=>
    {"Name"=>"NextState",
     "Input"=>{"foo"=>1, "result"=>{"foo"=>"bar", "bar"=>"baz"}},
     "Guid"=>"81de8e05-8118-49d0-b97e-779904fb07c3",
     "EnteredTime"=>"2024-05-24T14:34:29Z",
     "Output"=>{"foo"=>1, "result"=>{"foo"=>"bar", "bar"=>"baz"}},
     "NextState"=>nil,
     "FinishedTime"=>"2024-05-24T14:34:29Z",
     "Duration"=>0.348082391}

@Fryguy
Copy link
Member

Fryguy commented May 21, 2024

@GilbertCherrie I love the new icons, but there seems to be a spacing issue - I feel like the old icons looked proportionally better or perhaps aligned better? I can't quite put my finger on it.

@GilbertCherrie
Copy link
Member Author

@GilbertCherrie I love the new icons, but there seems to be a spacing issue - I feel like the old icons looked proportionally better or perhaps aligned better? I can't quite put my finger on it.

I can add padding to the right if they are too close

@GilbertCherrie
Copy link
Member Author

GilbertCherrie commented May 23, 2024 •
edited

@Fryguy I made all the requested changes if you can review again when you have time. The screenshot above shows the updated icon styles. Let me know if it's good now.

@miq-bot
Copy link
Member

miq-bot commented May 23, 2024

Checked commit GilbertCherrie@87d7593 with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
0 files checked, 0 offenses detected
Everything looks fine. ⭐

@jeffbonson jeffbonson merged commit a9b704b into ManageIQ:master May 24, 2024
15 checks passed
@GilbertCherrie GilbertCherrie deleted the fix_workflow_states_table branch May 24, 2024 12:54
@GilbertCherrie GilbertCherrie mentioned this pull request May 24, 2024
Merged
@Fryguy
Copy link
Member

Fryguy commented May 24, 2024

Backported to radjabov in commit dc52819.

commit dc5281932c9300f0f49f55512a8a6d4b1b18ec33
Author: Jeffrey bonson <jeffbonson@gmail.com>
Date:   Fri May 24 15:04:07 2024 +0530

    Merge pull request #9191 from GilbertCherrie/fix_workflow_states_table
    
    Add current state to workflow states table and format duration time
    
    (cherry picked from commit a9b704b98d530a01083ed8dc1cd3cf5eeeea5e47)

Fryguy pushed a commit that referenced this pull request May 24, 2024
@jeffbonson @Fryguy
Merge pull request #9191 from GilbertCherrie/fix_workflow_states_table
dc52819 
Add current state to workflow states table and format duration time

(cherry picked from commit a9b704b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fryguy Fryguy Fryguy left review comments

@agrare agrare agrare left review comments

@jeffibm jeffibm jeffibm approved these changes

@jeffbonson jeffbonson Awaiting requested review from jeffbonson

Assignees

@Fryguy Fryguy

@agrare agrare

@jeffibm jeffibm

Labels
enhancement radjabov/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Service Workflow States table doesn't show running states
6 participants
@GilbertCherrie @Fryguy @agrare @miq-bot @jeffbonson @jeffibm