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.

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

feat: Remove intermediate NodeExecutionsTable row content #75

Merged
merged 11 commits into from
Jun 22, 2020

Conversation

schottra
Copy link
Contributor

@schottra schottra commented Jun 18, 2020

TODOs and additional cleanup will happen in a separate PR

  • Implemented ExecutionDataCache, which is a shared data store used by components in the NodeExecutionsTable. This facilitates looking up the various entities (Nodes, Tasks, Workflows, WorkflowExecutions) that are needed at each level of the NodeExecutionsTable. This cache is provided via a context which sits above all components in the tree.
  • Updated all hooks/components in the ExecutionDetails and TaskExecutionDetails pages to load their data using an ExecutionDataCache, so that all related entities are stored and available in nested components.
  • Update the code for mapping NodeExecution -> DetailedNodeExecution (useDetailedNodeExecution[s]) to be pure functional transforms instead of working with Fetchables. The fetching/waiting logic has been moved up to the consuming components/hooks.
  • Moved contexts used in Execution components up to a common folder
  • Moved concept of DetailedNodeExecution down into the NodeExecutionsTable, since components above that do not need the extra information.
  • Updated NodeExecutionChildren to render "child groups" of NodeExecutions, instead of rendering either TaskExecution children or WorkflowExecution children. We still have differentiated behavior for the two cases, but both of them are now returning groups of NodeExecutions.
    • The primary use case for this is to group child NodeExecutions by attempt of the parent.
    • A previous PR implemented the basics of fetching child NodeExecutions. This one is making use of that functionality to actually render the children.
  • Moved column definitions for the NodeExecutionsTable into shared context. We don't need to be generating these definitions at every level of the table.
  • Fixed some flaky tests that have started consistently failing

Before:
image

After:
image

@codecov-commenter
Copy link

codecov-commenter commented Jun 18, 2020

Codecov Report

Merging #75 into dynamic-task-updates will increase coverage by 0.39%.
The diff coverage is 65.51%.

Impacted file tree graph

@@                   Coverage Diff                    @@
##           dynamic-task-updates      #75      +/-   ##
========================================================
+ Coverage                 61.66%   62.06%   +0.39%     
========================================================
  Files                       364      359       -5     
  Lines                      5713     5607     -106     
  Branches                    851      830      -21     
========================================================
- Hits                       3523     3480      -43     
+ Misses                     2190     2127      -63     
Impacted Files Coverage Δ
src/components/Cache/createCache.ts 76.92% <ø> (ø)
...nDetails/ExecutionNodeDetails/InputNodeDetails.tsx 0.00% <0.00%> (ø)
...Details/ExecutionNodeDetails/OutputNodeDetails.tsx 0.00% <0.00%> (ø)
.../ExecutionNodeDetails/TaskExecutionNodeDetails.tsx 0.00% <0.00%> (ø)
src/components/Executions/Tables/contexts.ts 100.00% <ø> (ø)
src/components/Executions/types.ts 100.00% <ø> (ø)
src/components/hooks/index.ts 100.00% <ø> (ø)
src/components/hooks/useTask.ts 45.45% <0.00%> (-14.55%) ⬇️
src/components/hooks/useWorkflow.ts 100.00% <ø> (ø)
src/components/hooks/useWorkflows.ts 50.00% <ø> (ø)
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d05ea7c...6705f62. Read the comment docs.

@schottra schottra requested review from podehnal and BobNisco June 18, 2020 21:31
src/components/Executions/Tables/contexts.ts Show resolved Hide resolved
src/components/Executions/contexts.ts Show resolved Hide resolved
src/components/Executions/useChildNodeExecutions.ts Outdated Show resolved Hide resolved
src/components/Executions/utils.ts Show resolved Hide resolved
src/components/hooks/useTask.ts Show resolved Hide resolved
Copy link

@podehnal podehnal left a comment

Choose a reason for hiding this comment

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

Overall LGTM 👍, just a couple of stylistic nits

@schottra schottra merged commit 2120f77 into dynamic-task-updates Jun 22, 2020
@schottra schottra deleted the execution-data-cache branch June 22, 2020 21:54
schottra added a commit that referenced this pull request Jun 29, 2020
* refactor: removing specialized rows and rendering only nodes

* refactor: moving contexts up to common folder

* refactor: use a data cache for nested node mapping

* refactor: update loading of workflow data

* fix: update usage of NodeExecutions in graph tab

* fix: update TaskExecutionDetails to use data cache

* fix: getting tests and stories working again

* chore: docs and cleanup

* test: use a more robust element query

* refactor: use filter instead of reduce

* docs: adding some missing function docs
schottra added a commit that referenced this pull request Jun 30, 2020
* refactor: removing specialized rows and rendering only nodes

* refactor: moving contexts up to common folder

* refactor: use a data cache for nested node mapping

* refactor: update loading of workflow data

* fix: update usage of NodeExecutions in graph tab

* fix: update TaskExecutionDetails to use data cache

* fix: getting tests and stories working again

* chore: docs and cleanup

* test: use a more robust element query

* refactor: use filter instead of reduce

* docs: adding some missing function docs
schottra added a commit that referenced this pull request Jun 30, 2020
* feat: adds more identifying information for node executions (#70)

* feat: show workflow node name beneath node execution ids

* feat: updating DetailsPanel info for NodeExecutions

* fix lint errors

* adding tests for new formatter function

* test: adding new test for NodeExecutionDetails

* test: adding test for new code in NodeExecutionsTable

* refactor: fetch child node executions for NodeExecution rows (#72)

* refactor: fetch child node executions to determine expandability

* fix: handling detection of children for sub-workflows as well

* fix: poor performance with object-hash on some identifiers

* docs: cleanup and docs for newly exposed functions

* test: ensure request config is used for all levels

* chore: remove unused import

* feat: Remove intermediate NodeExecutionsTable row content (#75)

* refactor: removing specialized rows and rendering only nodes

* refactor: moving contexts up to common folder

* refactor: use a data cache for nested node mapping

* refactor: update loading of workflow data

* fix: update usage of NodeExecutions in graph tab

* fix: update TaskExecutionDetails to use data cache

* fix: getting tests and stories working again

* chore: docs and cleanup

* test: use a more robust element query

* refactor: use filter instead of reduce

* docs: adding some missing function docs

* fix: cleanup for dynamic tasks refactoring (#76)

* test: creating dynamic task cases for NodeExecutionsTable stories

* fix: styling for child group labels

* fix: mock api context for NodeExecutionsTable stories

* test: mock nodeExecutionData endpoint

* chore: remove unused imports

* fix: extract nodes from subworkflows as well

* fix: adjust borders to make child groups more obvious

* refactor: checkpoint for getting the nesting styles correct

* refactor: adding logic for borders/spacing based on nesting/index

* fix: correct workflow execution table row styles
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants