Removed Started at date #1840

pnaik1 · 2023-09-21T11:47:01Z

Closes: #1543

Description

Removed the Started at date from Details tab of pipelineRun

When pipelineRuns are immediately triggerred the finished at first has N/A value later after completion of runs it gets it exact value

Screencast.from.2023-09-21.17-12-08.webm

How Has This Been Tested?

Check whether the Started at date is not present in the detail tab of pipelineRun
When creating a new piplineRun the finished at displays N/A instead of epoch value and later on gets the exact value

Test Impact

N/A

Request review criteria:

Self checklist (all need to be checked):

The developer has manually tested the changes and verified that the changes work
Commits have been squashed into descriptive, self-contained units of work (e.g. 'WIP' and 'Implements feedback' style messages have been removed)
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has added tests or explained why testing cannot be added (unit tests & storybook for related changes)

If you have UI changes:

Included any necessary screenshots or gifs if it was a UI change.
Included tags to the UX team if it was a UI/UX change (find relevant UX in the SMEs section).

After the PR is posted & before it merges:

The developer has tested their solution on a cluster by using the image produced by the PR to main

lucferbux · 2023-09-21T11:49:19Z

To add some context, I suggested @pnaik1 not adding testing for this PR, it can only be covered with integration/component testing, and we don't have anything in place for pipelines yet.

pnaik1 · 2023-09-21T11:49:44Z

the test for this pr should be covered with integration testing of pipelines, but it’s not already in place

manaswinidas

Works fine.

I think Started at was there for scheduled runs, but Created at and Started at were too close(8 seconds apart in the screenshot below). Correct me if I'm wrong. Is that why we chose to remove Started at altogether?

This screenshot is of a scheduled run(Run every minute). - what happens when we run every hour/every day? That's when the Started at and Created at may differ widely maybe.

Edit: Sorry I wanted to start a conversation, but clicked on "Request changes" instead.

lucferbux · 2023-09-22T07:55:11Z

Yes @manaswinidas you can check the whole conversation in the issue, but the main take with the outcome is summarized here: #1543 (comment)

We are removing Started at (scheduled at) cause it was too close to the other values.

DaoDaoNoCode

/lgtm
Tested, works well.

manaswinidas

/lgtm

Tested. Works fine.

frontend/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/PipelineRunTabDetails.tsx

christianvogt · 2023-10-18T20:42:00Z

frontend/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/PipelineRunTabDetails.tsx

-      value: asTimestamp(new Date(pipelineRunKF.scheduled_at || pipelineRunKF.created_at)),
+      key: 'Finished at',
+      value:
+        pipelineRunKF.finished_at === '1970-01-01T00:00:00Z'


Define a new constant and use it here: const epoch = new Date(0).toISOString();

Suggested change

pipelineRunKF.finished_at === '1970-01-01T00:00:00Z'

pipelineRunKF.finished_at === epoch

christianvogt · 2023-10-19T17:43:12Z

frontend/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/PipelineRunTabDetails.tsx

@@ -24,7 +24,7 @@ const PipelineRunTabDetails: React.FC<PipelineRunTabDetailsProps> = ({
  workflowName,
 }) => {
  const { namespace, project } = usePipelinesAPI();
-
+  const epoch = new Date(0).toISOString().slice(0, -5) + 'Z';


I'm sorry @pnaik1, My eyes deceived me when I compared your original string constant to the output of new Date(0).toISOString()! I didn't realize that the string values differed in the number of 0's ever so slightly. They still represent the same time however. Go back to using the string value you originally had: 1970-01-01T00:00:00Z.

The comment about creating a constant is to make it available for others in case they need to deal with this same use case in the future. Create the constant in concepts/pipelines/const.ts. Also, you can add a comment as to why this value exist. eg. A pipeline run that has not yet finished will have this value set as its 'finished_at' timestamp.
Let's also change the constant name to be more representative of how it's used by pipelines: INVALID_TIMESTAMP.

@christianvogt no worries , just to clarify or summarize the things , now I need to create a string constant value
(const INVALID_TIMESTAMP=1970-01-01T00:00:00Z)
in concepts/pipelines/const.ts ? and add the comments

Is there a reason we can't just write a utility to say isEmptyDateKF(resourceKF.finished_at) and it would just isolate this logic? I find it will be more pragmatic if we just lock away this logic away and stop writing inline functionality. Make it a common utility for all of the DS Pipelines effort.

The implementation of this method can be comparing a static string or doing proper date conversions, it doesn't matter either way as it just needs to get the job done.

Two solution under consideration

Using the constant

Implementing the utility function
@andrewballantyne @christianvogt which one do I need to go with?

Write a utility, use a constant or whatever you want inside it... stop making custom logic comparisons... if we can't use OOTB ISO format, don't write special code to do it. It's a kubeflow concept, write a utility for it.

a === b doesn't need to be anything more than a comparison... isoFormat.split.doOtherThing === b is now custom logic that you should not ever rewrite, so put it into a utility.

DRY with some intentional effort here not to make a need to refactor.

christianvogt · 2023-11-01T20:02:13Z

/lgtm
/approve

openshift-ci · 2023-11-01T20:02:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, DaoDaoNoCode, manaswinidas

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [christianvogt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot requested review from christianvogt and ppadti September 21, 2023 11:47

pnaik1 force-pushed the issue-1543 branch from 98e156d to 7270b66 Compare September 21, 2023 11:48

manaswinidas requested changes Sep 21, 2023

View reviewed changes

openshift-ci bot assigned manaswinidas Sep 21, 2023

lucferbux requested a review from manaswinidas September 22, 2023 07:55

DaoDaoNoCode approved these changes Sep 22, 2023

View reviewed changes

openshift-ci bot assigned DaoDaoNoCode Sep 22, 2023

openshift-ci bot added the lgtm label Sep 22, 2023

manaswinidas approved these changes Sep 22, 2023

View reviewed changes

alexcreasy requested changes Sep 28, 2023

View reviewed changes

frontend/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/PipelineRunTabDetails.tsx Outdated Show resolved Hide resolved

openshift-ci bot assigned alexcreasy Sep 28, 2023

openshift-ci bot removed the lgtm label Sep 28, 2023

alexcreasy added the pr/no-tests-allowed Added by an official approver - this PR is allowed no tests. Omitted, a test must accompany the PR label Sep 28, 2023

pnaik1 force-pushed the issue-1543 branch from 7270b66 to 94040a1 Compare October 16, 2023 10:23

christianvogt reviewed Oct 18, 2023

View reviewed changes

pnaik1 force-pushed the issue-1543 branch from 94040a1 to 4b83d24 Compare October 19, 2023 16:59

christianvogt reviewed Oct 19, 2023

View reviewed changes

Removed Started at date

d73145a

pnaik1 force-pushed the issue-1543 branch from 4b83d24 to d73145a Compare October 26, 2023 08:06

openshift-ci bot assigned christianvogt Nov 1, 2023

openshift-ci bot added the lgtm label Nov 1, 2023

openshift-ci bot added the approved label Nov 1, 2023

openshift-ci bot merged commit 7995cc7 into opendatahub-io:main Nov 1, 2023
3 checks passed

pnaik1 deleted the issue-1543 branch July 22, 2024 14:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removed Started at date #1840

Removed Started at date #1840

pnaik1 commented Sep 21, 2023 •

edited

Loading

lucferbux commented Sep 21, 2023

pnaik1 commented Sep 21, 2023

manaswinidas left a comment •

edited

Loading

lucferbux commented Sep 22, 2023

DaoDaoNoCode left a comment

manaswinidas left a comment

christianvogt Oct 18, 2023

christianvogt Oct 19, 2023

pnaik1 Oct 20, 2023

andrewballantyne Oct 20, 2023

pnaik1 Oct 25, 2023

andrewballantyne Oct 25, 2023

christianvogt commented Nov 1, 2023

openshift-ci bot commented Nov 1, 2023

	pipelineRunKF.finished_at === '1970-01-01T00:00:00Z'
	pipelineRunKF.finished_at === epoch

Removed Started at date #1840

Removed Started at date #1840

Conversation

pnaik1 commented Sep 21, 2023 • edited Loading

Description

How Has This Been Tested?

Test Impact

Request review criteria:

lucferbux commented Sep 21, 2023

pnaik1 commented Sep 21, 2023

manaswinidas left a comment • edited Loading

Choose a reason for hiding this comment

lucferbux commented Sep 22, 2023

DaoDaoNoCode left a comment

Choose a reason for hiding this comment

manaswinidas left a comment

Choose a reason for hiding this comment

christianvogt Oct 18, 2023

Choose a reason for hiding this comment

christianvogt Oct 19, 2023

Choose a reason for hiding this comment

pnaik1 Oct 20, 2023

Choose a reason for hiding this comment

andrewballantyne Oct 20, 2023

Choose a reason for hiding this comment

pnaik1 Oct 25, 2023

Choose a reason for hiding this comment

andrewballantyne Oct 25, 2023

Choose a reason for hiding this comment

christianvogt commented Nov 1, 2023

openshift-ci bot commented Nov 1, 2023

pnaik1 commented Sep 21, 2023 •

edited

Loading

manaswinidas left a comment •

edited

Loading