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.

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

feat: Tekton Results integration DEMO #3860

Closed
wants to merge 7 commits into from

Conversation

xinnjie
Copy link

@xinnjie xinnjie commented Dec 24, 2024

Changes

This is a demo for Tekton Results to integrate with Tekton Dashboard.
Related to tektoncd/results#82

Please focus on front-end part.

I want to know if there are any unwanted decisions made here .I'd like to propose a TEP about the integration in a few days.

Included functionality:

  • Add TaskRuns by Results in side navigation bar. TaskRuns by Results display all TaskRuns provided by Tekton Results API
  • TaskRun page data provideed by Tekton Results API, this page would be navigated to by clicking TaskRun in TaskRuns by Results
  • Logs in steps in provided by Tekton Results API

I will call API provided by Tekton Results ResultsAPI for short.

Impletation details

Resources URL mapping

There are about 3 types of resources in Tekton Results: Result, Record, Log, read Results Data Model for detail.

Here Record of TaskRun type and Log is involved.

tekton Dashboard Tekton Results
Record /apis/results.tekton.dev/v1alpha2/parents/{namespace}/results/{uid}/record/{uid} /namespaces/{namespace}/results/taskruns/{uid}/{uid}
Log /apis/results.tekton.dev/v1alpha2/parents/{namespace}/results/{uid}/logs/{uid} NA

Data accessing to ResultsAPI

Dashboard frontend sends requests to Dashboard backed, and Dashboard backend forwards all requests to Results backend.

How dashboard frontend queries resources of ResultAPI remains unclear and needs to be discussed. Implemantation in demo is merely for convenience. One of the goal should be taking advange of existing kubernates access control mechanism.

Need a plan design for how this is going to be, as mentioned in #3002 (comment)

New TaskRunsByResults Component Instead of reusing TaskRuns

  • Largely for implementation simplicity concern. Much easier than concerning Results resources and raw TaskRun recources at the same time.
  • The ResultsAPI can return significantly more TaskRuns than the API server, allowing for different pagination methods in the future.
  • TaskRuns from the ResultsAPI are immutable for end users, simplifying the TaskRunsByResults component compared to TaskRuns.
  • Implementation could be more progressive, avoiding changing TaskRuns behavior directly.

Basicly, I think a little boilerplate code here is better than mixing Results resources with TaskRun resources.

Snapshots:

image

image

Setup

In dashboard repo root directory:

scripts/installer install --enable-result

To enable log feature, checkout this branch in results repo root directory:

kubectl kustomize config/overlays/logs-local-db | ko apply -f -

More decisions to make

  • How dashboard front-end access ResultsAPI.
  • How to implement step-level logs: Currently, ResultsAPI provides TaskRun-level logs with a step prefix at the beginning of each line.

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

  • Docs included if any changes are user facing
  • Tests included if any functionality added or changed
  • Follows the commit message standard
  • Meets the Tekton contributor standards (including
    functionality, content, code)
  • Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
  • Release notes block below has been updated with any user facing changes (new features, significant UI changes, API changes, bug fixes, changes requiring upgrade notices or deprecation warnings)
  • Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

NONE

@tekton-robot tekton-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Dec 24, 2024
@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign briangleeson after the PR has been reviewed.
You can assign the PR to them by writing /assign @briangleeson in a comment when ready.

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

Copy link

linux-foundation-easycla bot commented Dec 24, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@tekton-robot tekton-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 24, 2024
@tekton-robot
Copy link
Contributor

Hi @xinnjie. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@xinnjie
Copy link
Author

xinnjie commented Dec 24, 2024

/hold

@tekton-robot tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 24, 2024
@xinnjie xinnjie force-pushed the feat-taskruns-results-demo branch from da5a3e7 to b3c2bea Compare December 24, 2024 11:32
@xinnjie
Copy link
Author

xinnjie commented Dec 24, 2024

You may find setup cumbersome. Here is a demonstration video.

demo.mov

@xinnjie xinnjie force-pushed the feat-taskruns-results-demo branch 2 times, most recently from da5a3e7 to b3c2bea Compare December 24, 2024 12:10
@AlanGreene
Copy link
Member

Thanks for the PR @xinnjie. It's great to see there's still interest from the community in making progress on integration of Tekton Results in the Dashboard.

That said, there are a number of issues with the approach taken in this PR. Fundamentally, we would much prefer to have the loading of data from either Results or from the k8s API happen transparently to the user in the API / data layer of the Dashboard as previously discussed in tektoncd/results#82. This means that there would be no separate 'TaskRuns by Results' pages, the existing pages would be populated with the data from whichever source is configured.

Before implementing the integration, it would be helpful to take a step back and look at a mapping between the views and functionality currently provided by the Dashboard and the API support currently available in Results.

As part of this, identify in detail any gaps on both sides. This would include things like:

  • the Dashboard taking advantage of the pagination support provided in Results to improve client performance by reducing the amount of data that needs to be loaded
  • or any additional functionality (e.g. improved filtering) that the integration could unlock
  • and on the Results side the lack of step-level log support as discussed on that issue and a number of previous issues here in the Dashboard repo, which has already been called out as a blocker that would need to be resolved first

Once we have that detailed set of desired features and functionality, we can explore the requirements for the UX design, supporting architecture and technical implementation details for any changes that may be required in the Dashboard to facilitate them.

There are a number of changes already planned for the Dashboard in early 2025 that may impact the approach taken for the integration of Results data into the UI, including:

I'm currently on vacation so if you have any further comments or questions I may not get a chance to respond until I return in the new year.

Thanks again for the PR, your detailed description, and the demo recordings.

@AlanGreene
Copy link
Member

Closing the PR, but feel free to continue discussion here.

@AlanGreene AlanGreene closed this Dec 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants