Eugene/task version #456

eugenejahn · 2022-05-09T21:16:50Z

TL;DR

made the entities component become more generic that can support Workflow, Task, and others
added the inputs and outputs to task details page
added the versions table in task details page
added the version details page
added task details link in node side panel

Task Details Page:
https://localhost.demo.nuclyde.io:3000/projects/flytesnacks/domains/development/tasks/athena.athena.print_athena_schema?duration=all

Task Version Details Page:
https://localhost.demo.nuclyde.io:3000/projects/flytesnacks/domains/development/task/athena.athena.print_athena_schema/version/katrinathena6

Task Details Link in Executions Details Page
https://localhost.demo.nuclyde.io:3000/projects/flytesnacks/domains/development/executions/ei0ud0veie?duration=all

Type

Bug Fix
Feature
Plugin

Are all requirements met?

Complete description

Tracking Issue

Remove the 'fixes' keyword if there will be multiple PRs to fix the linked issue

fixes https://github.com/flyteorg/flyteconsole/issues/448

Follow-up issue

NA
OR
https://github.com/flyteorg/flyte/issues/

codecov · 2022-05-09T21:38:13Z

Codecov Report

Merging #456 (9494fb8) into devmain (4e123d9) will increase coverage by 3.30%.
The diff coverage is 69.78%.

@@             Coverage Diff             @@
##           devmain     #456      +/-   ##
===========================================
+ Coverage    67.27%   70.58%   +3.30%     
===========================================
  Files          405      412       +7     
  Lines         9465     9601     +136     
  Branches      1606     1624      +18     
===========================================
+ Hits          6368     6777     +409     
+ Misses        3097     2824     -273

Impacted Files	Coverage Δ
...s/VersionDetails/EntityVersionDetailsContainer.tsx	`0.00% <0.00%> (ø)`
...rc/components/Entities/VersionDetails/constants.ts	`0.00% <0.00%> (ø)`
...es/zapp/console/src/components/Entities/strings.ts	`100.00% <ø> (+33.33%)`	⬆️
...ages/zapp/console/src/components/Theme/muiTheme.ts	`100.00% <ø> (ø)`
...es/zapp/console/src/components/common/BarChart.tsx	`88.88% <ø> (ø)`
...pp/console/src/components/common/ReactJsonView.tsx	`52.38% <ø> (ø)`
...ages/zapp/console/src/routes/ApplicationRouter.tsx	`0.00% <ø> (ø)`
packages/zapp/console/src/routes/components.ts	`0.00% <0.00%> (ø)`
...mponents/Executions/Tables/EntityVersionsTable.tsx	`53.57% <50.00%> (ø)`
...le/src/components/Entities/EntityDetailsHeader.tsx	`86.11% <66.66%> (+86.11%)`	⬆️
... and 51 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 4e123d9...9494fb8. Read the comment docs.

packages/zapp/console/src/components/Entities/EntityVersionDetails.tsx

packages/zapp/console/src/components/hooks/Entity/useEntityVersions.ts

packages/zapp/console/src/components/hooks/Entity/constants.ts

packages/zapp/console/src/components/Executions/Tables/WorkflowVersionsTable.tsx

packages/zapp/console/src/components/Entities/EntityDetailsHeader.tsx

packages/zapp/console/src/components/hooks/Entity/constants.ts

ursucarina · 2022-05-10T22:34:53Z

packages/zapp/console/src/components/hooks/Entity/useEntityVersions.ts

+  return usePagination<EntityType, IdentifierScope>(
+    { ...config, cacheItems: true, fetchArg: scope },
+    entityFunctions[(scope as Identifier)?.resourceType ?? Core.ResourceType.UNSPECIFIED]
+      .listEntity,


is listEntity always guaranteed to be present?

throw an error if not present

do we need to use xx.lisrEntity ?? someDefaultValue or wrap it with try/catch?

ursucarina · 2022-05-10T22:35:15Z

packages/zapp/console/src/models/__mocks__/taskData.ts

@@ -12,8 +13,9 @@ const decodedClosure = Admin.TaskClosure.create(
 const taskId: (name: string, version: string) => Identifier = (name, version) => ({
  name,
  version,
-  project: 'flyte',
-  domain: 'development',
+  project: testProject,


no-op: nice!

packages/zapp/console/src/components/Entities/generators.ts

ursucarina · 2022-05-10T22:40:51Z

packages/zapp/console/src/components/Entities/EntityExecutions.tsx

@@ -77,7 +79,7 @@ export const EntityExecutions: React.FC<EntityExecutionsProps> = ({
  return (
    <>
      <Typography className={styles.header} variant="h6">
-        All Executions in the Workflow
+        {t('allExecutionsChartTitle', entityStrings[id.resourceType])}


nit: @anrusina to weigh in here. I like going through strings to do this, however piecing strings like this is not the most localization friendly for the future. What do you think?

Agree with Carina! As resourceType will always be in English, so when strings would be translated it would contain a mix of two languages, which is not ok. Also often when you translate sentence to a different language the words location is changed, for example "yet" word in Russian is moved between you and slept:
ENG: Have you slept yet?
RUS: Ты уже спал?

Please use patternKey for strings like this. Sample label: t(patternKey('tableLabel', 'name')),
Here you will use as

{t(patternKey('allExecutionsChartTitle', entityStrings[id.resourceType]))}

In string file:

"allExecutionsChartTitle_": "Default string if entityStrings[id.resourceType] is null or undefined" "allExecutionsChartTitle_Workflow": "String for Workflow" "allExecutionsChartTitle_Tasks" : "String for Tasks"

I see that you probably used as example other string, found few other instances not created by you. Will cleanup them separately :)

ursucarina · 2022-05-10T22:45:07Z

Few comments regarding styling and code reuse

packages/zapp/console/src/components/Entities/EntityVersionDetails.tsx

packages/zapp/console/src/components/Entities/strings.ts

packages/zapp/console/src/components/Entities/test/EntityDetails.test.tsx

anrusina · 2022-05-11T20:25:21Z

packages/zapp/console/src/components/hooks/Entity/useEntityVersions.ts

+  return usePagination<EntityType, IdentifierScope>(
+    { ...config, cacheItems: true, fetchArg: scope },
+    entityFunctions[(scope as Identifier)?.resourceType ?? Core.ResourceType.UNSPECIFIED]
+      .listEntity,


do we need to use xx.lisrEntity ?? someDefaultValue or wrap it with try/catch?

anrusina

Mainly nits.
The string need to be updates, gave a sample implementation to Carina's comment

eugenejahn · 2022-05-11T21:02:28Z

Mainly nits. The string need to be updates, gave a sample implementation to Carina's comment

cool will fix it with next pr with the versions detail complete

anrusina

Mainly nits, One or two comments required fix.

It also has a big drop in test coverage.. Can you please work on increasing t for files you worked with? You can see missed checks in PR marked with notifications as:

Or you can run yarn test-coverage and than open coverage info in browser. For me it's file:///Users/narusina/prj/flyte/flyteconsole/.coverage/index.html - You can find you relative path.

Otherwise LGTM

packages/zapp/console/src/components/Entities/EntityDescription.tsx

packages/zapp/console/src/components/Entities/VersionDetails/EntityVersionDetails.tsx

anrusina · 2022-05-17T17:37:17Z

packages/zapp/console/src/components/Entities/strings.ts

-  launchStrings: (typeString: string) => `Launch ${startCase(typeString)}`,
-  noDescription: (typeString: string) => `This ${typeString} has no description.`,
-  noSchedules: (typeString: string) => `This ${typeString} has no schedules.`,
+  launchStrings_workflow: launchStrings('Workflow'),


nit: in real string files ready for localization we would never have functions inside, so ideally This Workflow has no schedules. should be used here and below instead of launchStrings('Workflow').
Mainly note for future, as we anyway would need tp go though all our files before we will be ready to add real localization support.

ohhh. I see now. that makes sense will fix it

...zapp/console/src/components/Executions/ExecutionDetails/NodeExecutionDetailsPanelContent.tsx

Signed-off-by: eugenejahn <[email protected]>

anrusina

LGTM, Nice Work

🚀

feat: task version details Signed-off-by: eugenejahn <[email protected]>

anrusina requested review from a team, ursucarina, anrusina and jsonporter and removed request for a team May 9, 2022 22:03

eugenejahn marked this pull request as ready for review May 10, 2022 18:24

olga-union reviewed May 10, 2022

View reviewed changes

packages/zapp/console/src/components/Entities/EntityVersionDetails.tsx Outdated Show resolved Hide resolved

ursucarina reviewed May 10, 2022

View reviewed changes

packages/zapp/console/src/components/hooks/Entity/constants.ts Outdated Show resolved Hide resolved

ursucarina reviewed May 10, 2022

View reviewed changes

packages/zapp/console/src/components/Entities/generators.ts Show resolved Hide resolved

ursucarina reviewed May 10, 2022

View reviewed changes

anrusina reviewed May 11, 2022

View reviewed changes

eugenejahn force-pushed the eugene/task-version branch from 4dc7232 to b76d4ea Compare May 16, 2022 22:37

anrusina reviewed May 17, 2022

View reviewed changes

anrusina force-pushed the devmain branch from bc2dd8e to 4e123d9 Compare May 17, 2022 23:46

eugenejahn force-pushed the eugene/task-version branch from 7bb8253 to 22e0130 Compare May 18, 2022 01:15

feat: task version details

9494fb8

Signed-off-by: eugenejahn <[email protected]>

eugenejahn force-pushed the eugene/task-version branch from 22e0130 to 9494fb8 Compare May 18, 2022 02:35

anrusina approved these changes May 18, 2022

View reviewed changes

eugenejahn merged commit caa222b into devmain May 18, 2022

eugenejahn deleted the eugene/task-version branch May 18, 2022 20:22

anrusina pushed a commit that referenced this pull request May 19, 2022

Eugene/task version (#456)

ace3158

feat: task version details Signed-off-by: eugenejahn <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eugene/task version #456

Eugene/task version #456

eugenejahn commented May 9, 2022 •

edited

Loading

codecov bot commented May 9, 2022 •

edited

Loading

ursucarina May 10, 2022

eugenejahn May 10, 2022 •

edited

Loading

anrusina May 11, 2022

ursucarina May 10, 2022

ursucarina May 10, 2022

anrusina May 11, 2022

anrusina May 11, 2022

ursucarina commented May 10, 2022

anrusina May 11, 2022

anrusina left a comment

eugenejahn commented May 11, 2022

anrusina left a comment

anrusina May 17, 2022

eugenejahn May 17, 2022

anrusina left a comment

Eugene/task version #456

Eugene/task version #456

Conversation

eugenejahn commented May 9, 2022 • edited Loading

TL;DR

Type

Are all requirements met?

Complete description

Tracking Issue

Follow-up issue

codecov bot commented May 9, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

eugenejahn May 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ursucarina commented May 10, 2022

Choose a reason for hiding this comment

anrusina left a comment

Choose a reason for hiding this comment

eugenejahn commented May 11, 2022

anrusina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anrusina left a comment

Choose a reason for hiding this comment

🚀

eugenejahn commented May 9, 2022 •

edited

Loading

codecov bot commented May 9, 2022 •

edited

Loading

eugenejahn May 10, 2022 •

edited

Loading