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.

Sign up for GitHub

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

Jump to bottom

ODC-7384: Update the PipelineRun details page to use Tekton Results API to load all the info #13313

Closed
wants to merge 1 commit into from
Closed

ODC-7384: Update the PipelineRun details page to use Tekton Results API to load all the info #13313

wants to merge 1 commit into from

Conversation

lokanandaprabhu
Copy link
Contributor

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Nov 8, 2023
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Nov 8, 2023 •
edited by openshift-ci bot

@lokanandaprabhu: This pull request references ODC-7384 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set.

In response to this:

Story : https://issues.redhat.com/browse/ODC-7384

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci openshift-ci bot added the component/pipelines Related to pipelines-plugin label Nov 8, 2023
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Nov 8, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: lokanandaprabhu
Once this PR has been reviewed and has the lgtm label, please assign karthikjeeyar, spadgett for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

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

@openshift-ci openshift-ci bot added the component/sdk Related to console-plugin-sdk label Nov 8, 2023
@lokanandaprabhu
Copy link
Contributor Author

/cc @jerolimov @vikram-raj

@lokanandaprabhu lokanandaprabhu force-pushed the feature/ODC-7384 branch 2 times, most recently from bb8a1a1 to d374a91 Compare November 8, 2023 13:29
@lokanandaprabhu lokanandaprabhu changed the title ODC-7384: Update the PipelineRun details page to use Tekton Results API to load all the info [WIP] ODC-7384: Update the PipelineRun details page to use Tekton Results API to load all the info Nov 8, 2023
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 8, 2023
@jerolimov
Copy link
Member

/uncc christianvogt rottencandy

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 12, 2023
@lokanandaprabhu lokanandaprabhu changed the title [WIP] ODC-7384: Update the PipelineRun details page to use Tekton Results API to load all the info ODC-7384: Update the PipelineRun details page to use Tekton Results API to load all the info Nov 13, 2023
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 13, 2023
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 13, 2023
@openshift-ci openshift-ci bot added the kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated label Nov 13, 2023
@lokanandaprabhu
Copy link
Contributor Author

Hi @vikram-raj / @jerolimov

I updated the PR. PTAL. Thanks.

@lokanandaprabhu lokanandaprabhu mentioned this pull request Nov 14, 2023
Merged
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Nov 14, 2023

@lokanandaprabhu: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-gcp-console 5e5688a link true /test e2e-gcp-console

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

vikram-raj
vikram-raj reviewed Nov 15, 2023
View reviewed changes
Copy link
Member

@vikram-raj vikram-raj left a comment •
edited

Choose a reason for hiding this comment

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

internet.network.call.on.PLR.details.page.mov

I noticed an issue. On the details page, there is a continuous network call for results API.

karthikjeeyar
karthikjeeyar reviewed Nov 15, 2023
View reviewed changes
Copy link
Contributor

@karthikjeeyar karthikjeeyar left a comment

Choose a reason for hiding this comment

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

Fetching TektonResult CR, tekton results API service and the actual tekton-results pipelinerun API call is being called in infinitely in the pipelinerun details page.

image

frontend/packages/pipelines-plugin/src/components/pipelineruns/PipelineRunDetailsPage.tsx
Comment on lines +50 to +63
const [pipelineRun, pipelineRunLoaded] = useK8sWatchResource<FirehoseResult<K8sResourceKind>>({
kind: referenceForModel(PipelineRunModel),
namespace,
name,
isList: false,
});
const [TRPlrs, TRPlrsLoaded] = useTRPipelineRuns(namespace, {
selector: { filterByName: name },
});

React.useEffect(() => {
setData(pipelineRun || TRPlrs[0]);
setLoaded(pipelineRunLoaded || TRPlrsLoaded);
}, [TRPlrs, pipelineRun, pipelineRunLoaded, TRPlrsLoaded]);
Copy link
Contributor

Choose a reason for hiding this comment

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

You can extract this logic as a separate hook called usePipelineRun, so that this hook can be used elsewhere in the codebase as well.

frontend/packages/pipelines-plugin/src/components/pipelineruns/PipelineRunDetailsPage.tsx
obj={{
data,
loaded,
loadError: undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we pass the actual error object here?

@karthikjeeyar
Copy link
Contributor

In pipelinerun details page, we will be fetching all the taskruns as well. So when you delete a pipelienrun (which will inturn delete all the taskruns automatically from K8s cluster) and then go to that deleted details page you can see the visualization with wrong task status, we need to use tekton-results for fetching the taskruns here as well.

image

@lokanandaprabhu
Copy link
Contributor Author

Closing this PR since it is handled in PR - #13328

/close

@openshift-ci openshift-ci bot closed this Nov 16, 2023
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Nov 16, 2023

@lokanandaprabhu: Closed this PR.

In response to this:

Closing this PR since it is handled in PR - #13328

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vikram-raj vikram-raj vikram-raj left review comments

@karthikjeeyar karthikjeeyar karthikjeeyar left review comments

@jerolimov jerolimov Awaiting requested review from jerolimov

Assignees
No one assigned
Labels
component/pipelines Related to pipelines-plugin component/sdk Related to console-plugin-sdk jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@lokanandaprabhu @openshift-ci-robot @jerolimov @karthikjeeyar @vikram-raj @openshift-merge-robot