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

TEP-0160 Enhance Tekton Results CLI #1179

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

divyansh42
Copy link
Member

@divyansh42 divyansh42 commented Feb 10, 2025

Proposing a TEP to enhance the existing functionalities of the Tekton Results CLI

Inspired by the kubectl plugin developed by @sayan-biswas ❤️

/kind tep

@tekton-robot tekton-robot added the kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). label Feb 10, 2025
@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 10, 2025
@divyansh42 divyansh42 force-pushed the results-cli-tep branch 2 times, most recently from 7bdbe76 to c1f3a82 Compare February 11, 2025 08:31

### Backward Compatibility

Existing `tkn-results` CLI commands should not break. We should add a deprecation message to the old commands to inform users about the new structure and provide guidance on transitioning to the updated CLI.
Copy link
Contributor

Choose a reason for hiding this comment

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

We can probably hide these commands under debug or something. I found these to be helpful while debugging sometime.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we will not remove the existing commands, as users might be relying on those commands.

@divyansh42
Copy link
Member Author

@sayan-biswas could you please review the TEP once? Thank you.

```sh
tkn-results pr ls -n <namespace> # Get list of PipelineRuns in a namespace
tkn-results tr ls -n <namespace> # Get list of TaskRuns in a namespace
```

Choose a reason for hiding this comment

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

@divyansh42, we have seen frequent usage where as a Tekton admin, i may want to observe a pattern of failures for a specific group of pipeline runs. Can we include option to fetch pr/tr list based on some selectors(labels) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@anithapriyanatarajan thanks for taking a look. I am planning to add a filtering option in the next iteration.
But this sounds like a good filter to add. I will work further on this.

Comment on lines 95 to 112
```sh
tkn-results pr ls -n <namespace> # Get list of PipelineRuns in a namespace
tkn-results tr ls -n <namespace> # Get list of TaskRuns in a namespace
```

#### Describing Runs

```sh
tkn-results pr describe <pr-name> -n <namespace> # Get details of a specific PipelineRun
tkn-results tr describe <tr-name> -n <namespace> # Get details of a specific TaskRun
```

#### Fetching Logs

```sh
tkn-results pr logs <pr-name> -n <namespace> # Fetch logs of a specific PipelineRun
tkn-results tr logs <tr-name> -n <namespace> # Fetch logs of a specific TaskRun
```

Choose a reason for hiding this comment

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

I imagine these would be mostly equivalent to tkn pr ls, tkn pr describe, tkn pr logs?

It would be great if they supported (most of) the same CLI flags, so that I could just take a regular tkn ... command, change it to tkn results ... and have it work. But I wouldn't necessarily expect that to be the case in the first iteration.

Choose a reason for hiding this comment

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

Eventually, if the tkn-results commands become fully compatible with the equivalent tkn commands, maybe the the regular commands could become results-aware?

So I wouldn't need to try both tkn pr logs and tkn results pr logs -- tkn pr logs would "just work"?

Copy link
Member Author

Choose a reason for hiding this comment

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

I imagine these would be mostly equivalent to tkn pr ls, tkn pr describe, tkn pr logs?

Thanks for taking a look @chmeliik
Yes, that's what we have planned right now. I should eventually add those flags based on the requirements.

Eventually, if the tkn-results commands become fully compatible with the equivalent tkn commands, maybe the the regular commands could become results-aware?

So I wouldn't need to try both tkn pr logs and tkn results pr logs -- tkn pr logs would "just work"?

I agree with your point, I think that will be a part of long-term goals for the tkn

Copy link

Choose a reason for hiding this comment

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

Would tkn results only look at the stored results or would it also be aware of the data in etcd? If the subcommand can work for either, then feature parity definitely makes sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think as of now, tkn-results will look for data in the stored results but I think moving forward long term goal would be to have the functionality in tkn where it can look for data stored in etcd as well as the results.
cc @vdemeester

Copy link

@enarha enarha left a comment

Choose a reason for hiding this comment

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

Is the code going to be maintained as part of the results repo (https://github.com/tektoncd/results) or as a separate repo?

@divyansh42
Copy link
Member Author

Is the code going to be maintained as part of the results repo (https://github.com/tektoncd/results) or as a separate repo?

Will be maintained as a part of results repo (https://github.com/tektoncd/results)

@divyansh42 divyansh42 changed the title TEP-0157 Enhance Tekton Results CLI TEP-0160 Enhance Tekton Results CLI Feb 20, 2025
@divyansh42 divyansh42 force-pushed the results-cli-tep branch 3 times, most recently from 8264257 to efb89a4 Compare February 26, 2025 11:17
@afrittoli
Copy link
Member

The lint job fails because the TEP table has not been updated to match.

@khrm
Copy link
Contributor

khrm commented Mar 4, 2025

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 4, 2025
@tekton-robot tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 5, 2025
@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 ask for approval from khrm after the PR has been reviewed.

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

@divyansh42
Copy link
Member Author

Added details related to the pagination support

@khrm
Copy link
Contributor

khrm commented Mar 5, 2025

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 5, 2025
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

Few comments, but otherwise Looks Good To Me 😉


## Summary

`Tekton Results` provides a mechanism to store and query the execution history of `PipelineRuns` and `TaskRuns`. However, the [existing CLI](https://github.com/tektoncd/results/tree/main/pkg/cli) for interacting with Results was not user-friendly. This proposal introduces a dedicated `CLI tool` that simplifies querying and managing Tekton Results, allowing users to retrieve, inspect, and analyze results efficiently. Additionally, this new CLI can be used as a standalone tool or as a plugin to the `tkn` CLI, ensuring seamless integration with existing Tekton workflows.
Copy link
Member

Choose a reason for hiding this comment

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

nit: we could use present tense (was not user-friendly -> is not user-friendly)


### Non-Goals

This proposal is not intended to modify the archived PipelineRuns and TaskRuns
Copy link
Member

Choose a reason for hiding this comment

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

I would also add another non-goal here about not integrating it with tkn (yet)

Copy link
Member

Choose a reason for hiding this comment

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

Like "This proposal doesn't explore possible deep integration with tkn (e.g. transparently query results when doing a tkn pr list), it would be another proposal later once we are happy with this cli".

Comment on lines +90 to +104
#### Configuration

The configuration will allow users to provide the following options:
- **Host**: The host address for the client to connect.
- **Token**: The authentication token for secure API access.
- **API Path**: The specific API endpoint path.
- **Skip TLS verification**: Skip TLS verification for the API access (default: false)

These values can be automatically fetched and will have default values when possible.

```sh
tkn-results config set # Set authentication and configuration parameters
tkn-results config reset # Reset the configuration to default values
tkn-results config view # View the current configuration
```
Copy link
Member

Choose a reason for hiding this comment

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

I guess this is stored in a file. We need to make sure it's possible for users to have multiple configuration files and switch from one to the other using an environment variable (à-la KUBECONFIG) or a flag (--config ?)

Copy link
Member Author

@divyansh42 divyansh42 Mar 6, 2025

Choose a reason for hiding this comment

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

There is an option to provide --kubeconfig flag which will allow users to use different config file instead of the default one.
If we want we can also include environment variable as well.

Copy link
Member

Choose a reason for hiding this comment

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

Isn't it the tkn-results configuration more than just the kubeconfig ?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it is all kubeconfig.
By default, it will fetch from kubeconfig (which will show by default in the config command) and the user provided data will also be stored in Extension in kubeconfig

Signed-off-by: divyansh42 <[email protected]>
@tekton-robot tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 7, 2025
@tekton-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@divyansh42
Copy link
Member Author

Few comments, but otherwise Looks Good To Me 😉

Thanks for the suggestions, I have added a commit to address those.
For the last one, I will make the changes once we conclude the discussion.

I Will squash the commit before the merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants