Follow up misc fixes for pipeline version feature #2352
Conversation
|
|
||
| type PipelineRunDetailsVersionContextProps = { | ||
| children: React.ReactNode; | ||
| resource?: PipelineRunJobKF | PipelineRunKF | null; |
There was a problem hiding this comment.
nit: Don't we only want this context used if there's a resource provided? Seems like this should be marked as required.
There was a problem hiding this comment.
Yes, I need to do some refactor on the pipeline details page because the run is from const run = runResource?.run;, which could be undefined and the early exit should be checked. I will change that.
| children, | ||
| resource, | ||
| }) => { | ||
| const versionRef = getPipelineVersionResourceRef(resource); |
There was a problem hiding this comment.
It looks like this context is only accessed in 1 place, where this version data is needed, I'm wondering if it makes more sense to simply store the "run" object for this context and instead call it PipelineRunDetailsContext. Since I think keeping track of a run from a couple different locations is its main purpose.
| @@ -85,7 +86,7 @@ const PipelineRunDetails: PipelineCoreDetailsPageComponent = ({ breadcrumbPath, | |||
| } | |||
|
|
|||
| return ( | |||
| <> | |||
| <PipelineRunDetailsVersionContextProvider resource={run}> | |||
There was a problem hiding this comment.
I think we can limit this to wrap around PipelineRunDrawerBottomContent only since that's the only place the context seems to be used.
There was a problem hiding this comment.
Same goes for the PipelineRunJobDetails usage here.
| const usePipelineRunVersionInfo = ( | ||
| resource: PipelineRunKF | PipelineRunJobKF | null, | ||
| ): { | ||
| version?: PipelineVersionKF; |
There was a problem hiding this comment.
Don't we always return version and error, they're just potentially undefined/null? We only want to make props optional if they're truly optional, not just to account for undefined values.
DaoDaoNoCode
force-pushed
the
jira-rhoaieng-1749
branch
from
8b61eff
to
2917e3f
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: jpuzz0 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 |
openshift-merge-bot
bot
merged commit 6e090e2
into
opendatahub-io:f/pipelines-enhancement
JIRA: https://issues.redhat.com/browse/RHOAIENG-1749
Description
This PR did the following things:
disableRowRenderSupportsetView all runsfor pipeline rowHow Has This Been Tested?
Test Impact
N/A, will add later.
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main