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
TaskRun details and log page #6851
TaskRun details and log page #6851
Conversation
5ada0b3
to
ea246a8
Compare
props.obj.status?.podName ? ( | ||
<TaskRunLog {...props} /> | ||
) : ( | ||
<StatusBox loadError="Pod not found" label="TaskRun Log" /> |
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.
Why can't we have this check in the above function and early exit if podName is not available 🤔
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 works, but it fails lint check when there is an if
block before the useK8sGet
Hook
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.
Best to name this wrapper component and export that as default.
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.
named as wrapper
/kind feature |
obj.status.podName, | ||
obj.metadata.namespace, | ||
); | ||
if (podLoaded) { |
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.
if (podLoaded) { | |
if (podLoaded && !podError) { |
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.
updated this as well as named to TaskRunLogWrapper
props.obj.status?.podName ? ( | ||
<TaskRunLog {...props} /> | ||
) : ( | ||
<StatusBox loadError="Pod not found" label="TaskRun Log" /> |
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.
Best to name this wrapper component and export that as default.
ea246a8
to
4705e16
Compare
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.
@abhinandan13jan Logs doesn't seem to be streaming. It just says loading and when step is changed from the dropdown. There is an error
But it streams fine in the Pipeline run logs
}; | ||
|
||
// to safeguard against cases where Pod is yet to be assigned or is removed | ||
const TaskRunLogWrapper = (props: TaskRunLogProps) => |
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 a Functional Component. Should type it properly
const TaskRunLogWrapper = (props: TaskRunLogProps) => | |
const TaskRunLogWrapper: React.FC<TaskRunLogProps> = (props) => |
<Status status={pipelineRunFilterReducer(taskRun)} /> | ||
</dd> | ||
</dl> | ||
<PipelineRunDetailsErrorLog pipelineRun={taskRun} /> |
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.
These two pipelineRun specific functions called in the context of taskRun
looks odd to me. If taskRun has more specific type definition, then this will cause lint errors and confusions in future. It will be good to refactor these to a generic function calls. WDYT?
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.
I've updated such that PLR and TR uses separate utils and point to a common component
Strange, I find it to work the same as Pod log |
4705e16
to
78e130c
Compare
pipelineRun: PipelineRun; | ||
pipelineRun?: PipelineRun; | ||
taskRun?: K8sResourceKind; |
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.
Maybe we just simplify this to a single K8sResourceKind
or even better down to just namespace
?
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.
simplified to namespace for all cases
describe('TaskRunDetailsPage', () => { | ||
const pipelineRunWrapper = shallow(<TaskRunDetails {...TaskDetailsProps} />); | ||
it('Renders a SectionHeading', () => { | ||
expect(pipelineRunWrapper.find(SectionHeading).exists()); | ||
}); | ||
it('Renders a Resource Summary', () => { | ||
expect(pipelineRunWrapper.find(ResourceSummary).exists()); | ||
}); | ||
it('Renders a Status', () => { | ||
expect(pipelineRunWrapper.find(Status).exists()); | ||
}); | ||
it('Renders a Task Run Log snippet', () => { | ||
expect(pipelineRunWrapper.find(TaskRunDetailsErrorLog).exists()); | ||
}); | ||
}); |
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.
Testing against static content won't save us from anything. The best it can do is fail a test when we change the layout... there isn't any conditional here that we would be concerned about. Ie, no logic breaks can cause these tests to fail.
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 just based on current acceptance criteria. Just to make sure, the code adheres to the AC
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.
Unit tests guard against accidental breaks in logic.
Eg. A unit test for add5()
would be add5(5).toBe(10)
... you could test positive numbers, negative numbers, strings (should return NaN), objects, etc, etc...
If you wrap your add5()
:
function getOffsetNumber() {
return add5(37);
}
This function doesn't get you much but it allows the caller not to worry about providing this static 37
(this is an overly simple example). A testing of this function doesn't mean much. getOffsetNumber().toBe(42)
... how will this test break? If we choose 37
is not a valid constant anymore? So we just change the test and the code at the same time and nothing super shocking here happens. No assistance is really provided by our test, no assurances we don't accidentally break.
Tests are so you can go into add5
and change the fact that it doesn't add 5 anymore, it subtracts 8 or does something else (uses math.random for instance). Zero inputs or inputs that don't change the output are not test worthy scenarios. They just explicitly tie our tests to our implementation and thus makes us update one more place when we need to change something. It doesn't guard against breaks.
With this mental model, React Components have a similar structure with 1 extra condition. We only shallowly look at the structure of a component. So if your React Component does not take any props or the props are not conditional rendering props but just pass-thru props, then there are no tests needing to be written.
So in summary, because I ranted a bit here. Don't test a component that does not use conditional rendering as you're not really writing a test, you're writing just 1 more spot to update if we need to change anything.
import { PodModel } from '@console/internal/models'; | ||
|
||
export type TaskRunLogProps = { | ||
obj: K8sResourceKind; |
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.
obj: K8sResourceKind; | |
obj: TaskRunKind; |
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.
updated
78e130c
to
d32ca0c
Compare
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.
I observe the same issue as sahil mentioned above.When you create a taskRun, logs are not streaming correctly.
@@ -58,3 +58,8 @@ export const pipelineResourceTypeFilter = (filters, pipelineResource): boolean = | |||
const type = pipelineResourceFilterReducer(pipelineResource); | |||
return filters.selected.has(type) || !_.includes(filters.all, type); | |||
}; | |||
|
|||
export const taskRunFilterReducer = (taskRun): string => { |
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.
export const taskRunFilterReducer = (taskRun): string => { | |
export const taskRunFilterReducer = (taskRun: TaskRunKind): string => { |
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.
cyclic dependence will be there
@@ -58,3 +58,8 @@ export const pipelineResourceTypeFilter = (filters, pipelineResource): boolean = | |||
const type = pipelineResourceFilterReducer(pipelineResource); | |||
return filters.selected.has(type) || !_.includes(filters.all, type); | |||
}; | |||
|
|||
export const taskRunFilterReducer = (taskRun): string => { | |||
const status = pipelineRunStatus(taskRun); |
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.
pipelineRunStatus
function should be also typed to accept PipelineRun
| TaskRunKind
as arguments
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.
There's one issue with that. It's a cyclic dependence and pipeline-filter-reducer doesn't have types. It should be a separate issue to refactor Types
out of pipeline-augment. cc: @andrewballantyne
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.
I would prefer here to not reuse pipelineRunStatus
at all and to not mix PipelineRun and TaskRun objects. I personally would prefer in this case "duplicated" code for both types instead of methods which supports both.
But that's up to you. But we should at least add some types here. +1 for that.
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.
Yeah, types are annoying in this file. So it should be handled by the work to take Pipelines out of DevConsole package
const TaskDetailsProps: Props = { | ||
obj: { | ||
kind: 'TaskRun', | ||
metadata: { name: 'abhi', namespace: 'abhi1' }, | ||
status: { | ||
completionTime: '2019-10-29T11:57:53Z', | ||
conditions: [ | ||
{ | ||
lastTransitionTime: '2019-10-29T11:57:53Z', | ||
reason: 'Failed', | ||
status: 'True', | ||
type: 'Failed', | ||
}, | ||
], | ||
}, | ||
}, | ||
}; | ||
|
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.
can we please move this to a test-data file ? and reuse in other spec files.
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.
moved
import * as React from 'react'; | ||
import { PodLogs } from '@console/internal/components/pod-logs'; | ||
import { StatusBox } from '@console/internal/components/utils/status-box'; | ||
import { shallow } from 'enzyme'; |
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.
nit: sort imports
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.
updated
d32ca0c
to
127ffb5
Compare
Fixed it with useK8sWatchResource hook |
127ffb5
to
70c915a
Compare
Will all of the columns be added on the list view? Right now I only see namespace and created, but there should be others included as seen in this PR - #6840 (comment) I'm not sure if these designs will automatically update based on Deb's PR or not... |
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.
Based on @siamaksade comment ...
Strange, I find it to work the same as Pod log
@abhinandan13jan If possible, I'd suggest that we use the same component from the Log tab in PipelineRun here, but just not show the left nav piece with the task list
I think this was a miscommunication because we discussed "using conventions" :)
@bgliwa01 Yes, the list view will be updated as per @debsmita1 's PR. You can ignore the list view shown in my screenshots :) |
70c915a
to
70aaab8
Compare
Hi @serenamarie125 !! I've updated to use the Multistreaming Log |
Verified locally, works fine |
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
@abhinandan13jan looks like, it needs rebase |
ca4221e
to
0d31381
Compare
import LogSnippetBlock from './LogSnippetBlock'; | ||
|
||
type PipelineStatusLogProps = { | ||
pipelineRun: PipelineRun; | ||
logDetails; |
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.
Need a type
0d31381
to
fc7611f
Compare
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 good, just need to use the TaskRunKind
export type TaskRunStatus = { | ||
podName?: string; | ||
conditions: Condition[]; | ||
}; | ||
|
||
export type TaskRunData = K8sResourceKind & { | ||
status: TaskRunStatus; | ||
}; |
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.
Should just add these to the TaskRunKind
...
|
||
export interface TaskRunDetailsProps { | ||
obj: K8sResourceKind; | ||
obj: K8sResourceKind & { status: TaskRunStatus }; |
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.
obj: K8sResourceKind & { status: TaskRunStatus }; | |
obj: TaskRunKind; |
import { CombinedErrorDetails, TaskRunData } from '../../pipelineruns/logs/log-snippet-types'; | ||
import { taskRunSnippetMessage } from '../../pipelineruns/logs/log-snippet-utils'; | ||
|
||
export const getTRLogSnippet = (taskRun: TaskRunData): CombinedErrorDetails => { |
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.
export const getTRLogSnippet = (taskRun: TaskRunData): CombinedErrorDetails => { | |
export const getTRLogSnippet = (taskRun: TaskRunKind): CombinedErrorDetails => { |
fc7611f
to
9821974
Compare
/retest |
1 similar comment
/retest |
9821974
to
2baf77f
Compare
/retest |
2baf77f
to
1e2f3d9
Compare
/test backend |
Verified locally, works fine |
/retest Please review the full test history for this PR and help us cut down flakes. |
export type TaskRunStatus = { | ||
completionTime?: string; | ||
conditions: Condition[]; | ||
podName?: string; | ||
startTime?: string; | ||
steps?: PLRTaskRunStep[]; | ||
}; | ||
|
||
export type TaskRunData = K8sResourceKind & { | ||
status: TaskRunStatus; | ||
}; |
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.
Failed to remove this code 😄 Please trim it out so we don't have confusion on which to use.
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.
yup.. updated..thanks
/lgtm cancel |
/approve Remove the unused code and we should be good to go. |
1e2f3d9
to
ba944b6
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinandan13jan, andrewballantyne, bgliwa01, jerolimov, karthikjeeyar 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:
Approvers can indicate their approval by writing |
Addresses
https://issues.redhat.com/browse/ODC-4801
Screenshot
Tests
Added two test suites, TaskRunDetails and TaskRunLogs
Browser conformance
chrome