-
Notifications
You must be signed in to change notification settings - Fork 74
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
tkn-results: Add CLI for listing results and records. #100
Conversation
Is the intention for this to be a CLI for testing/playing around with the API? If so it probably doesn't need super thorough tests, and should just be documented as test-only. If it's not, we probably need to have a longer discussion with the tkn folks about what the relationship is between the two. |
tools/tkn-results/README.md
Outdated
## Installation | ||
|
||
``` | ||
$ GOBIN=${TKN_PLUGINS_DIR:-"${HOME}/.config/tkn/plugins"} go install github.com/tektoncd/experimental/tkn-resolve |
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.
Is github.com/tektoncd/experimental/tkn-resolve
the right path?
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.
Good catch! 😅 Fixed.
Short term this is to let users have a nicer mechanism to work with the Results API without needing to jump through grpc_cli hoops. There's a blurry line here between a test-only tool and the recommended way to see results on a CLI. I agree that we should have a longer discussion about how results fits in to the existing tkn UX for pipelineruns/taskruns (particularly since results is an optional component), but for now our hope is to start prototyping with a plugin. |
} | ||
) | ||
|
||
// Execute executes the root command. |
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.
Typo?
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.
Removed.
} | ||
) | ||
|
||
// Execute executes the root command. |
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.
Same here
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.
Removed.
This adds a new tkn plugin for interacting with the Results API. For now this only implements the list methods for results and records. I wasn't sure what the best way to go about testing the whole CLI, since most of the complexity is setting up the client. We might want to consider adding this as a part of the e2e tests in a future PR.
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: XinruZhang 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 |
This adds a new tkn plugin for interacting with the Results API.
For now this only implements the list methods for results and records.
I wasn't sure what the best way to go about testing the whole CLI, since
most of the complexity is setting up the client. We might want to
consider adding this as a part of the e2e tests in a future PR.
Implements #81