Basic execution plugin module by vdemeester · Pull Request #1178 · tektoncd/cli

vdemeester · 2020-09-18T14:56:54Z

Changes

This adds a basic execution plugin model, very simple to be honest. Most of the work has been done by @sthaha. In a gist this is how it works

If the specified subcommand doesn't exists, we check if a command
with the subcommand prefixed by tkn- exists in the
$HOME/.config/tkn/pulgins (for foo we look for tkn-foo).
The default folder ($HOME/.config/tkn/plugins) can be overriden
with TKN_PLUGINS_DIR.
If the binary doesn't exists, we proceed as usual
If the binary exists, we execute it "à-la" exec …, aka we give full hand to the binary.

$ ls ~/.config/tkn/plugins/tkn-*
/home/vincent/.config/tkn/plugins/tkn-bar  /home/vincent/.config/tkn/plugins/tkn-foo
$ tkn foo is bar --hello
args is bar --hello
fooo
bar
$ tkn bar bar
/usr/bin/env: ‘invalidcommand’: No such file or directory

What may need to be enhanced :

~~This needs quick & dirty tests~~
This doesn't pass any global flags (value or as arguments — tkn doesn't have any it seems) — any "client handling" is on the plugin binary to handle
This doesn't take into account help — tkn foo --help would call tkn-foo --help, but tkn help won't show foo as subcommand. This can probably be taken care of in follow-ups ?
~~This only look into PATH. Maybe we want to have some "standard"~~

Carrying #1136.
Fixes: #1039
Signed-off-by: Sunil Thaha sthaha@redhat.com

/cc @tektoncd/cli-maintainers

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Run the code checkers with make check
Regenerate the manpages, docs and go formatting with make generated
Commit messages follow commit message best practices

See the contribution guide
for more details.

Release Notes

`tkn` now support a basic plugin system. Any command prefixed by `tkn-` in the `PATH` can be used as a `tkn` subcommands (e.g. `tkn-foo` can be used as `tkn foo`).

sthaha · 2020-09-20T12:04:42Z

How about fmt.Fprintf(os.Stderr, ...

sthaha · 2020-09-20T12:08:19Z

why 254? should it be 127? Command not found or not in path ? doesn't 128+<exit-code> have a different meaning? 🤔

Random number 😅 I'll update to 127 😝

sthaha · 2020-09-20T12:10:40Z

we should write a test for this shouldn't we? ' at least a shell test ?

@sthaha yeah it's on the desc of the PR 😁

sthaha · 2020-09-20T12:11:28Z

won't this crash if cmd == nil ?

@sthaha right, it will, but cmd is never nil. But this is an artefact of a refactoring I tried 😅 it should be tkn.Execute(), I'll update 😅

sm43 · 2020-09-21T15:55:43Z

Why tkn.Find(args) doesn't return an error when an invalid command is passed?
I tried with another CLI Find returns err when the command is invalid, it happens only with tkn. 😄

@sm43 my guess is the suggest part but I am not sure 😓

vdemeester · 2020-10-30T10:56:13Z

This should be ready to be reviewed. Fixed some problems and added a small e2e test 🙃

vdemeester · 2020-10-30T13:52:39Z

/test pull-tekton-cli-integration-tests

vdemeester · 2020-10-30T14:18:21Z

TestPipelineRunLogE2E: pipelinerun_test.go:55: could not get pipelinerun: conversion webhook for tekton.dev/v1beta1, Kind=PipelineRun failed: conversion failed to version v1alpha1 for type [kind=PipelineRun group=tekton.dev version=v1beta1] -  the specified field/section is not available in v1alpha1

🤔

sthaha

looks good to me except of the the unrelated CI failure.

vdemeester · 2020-11-02T14:16:54Z

/retest

sthaha

/lgtm

danielhelfand · 2020-11-10T17:08:37Z

/retest

pradeepitm12 · 2020-11-10T17:35:19Z

/retest

danielhelfand · 2020-11-11T03:32:45Z

/retest

pradeepitm12 · 2020-11-11T05:46:34Z

/retest

danielhelfand · 2020-11-11T16:56:34Z

With #1249 merged, should resolve EventListener test failures

vdemeester · 2020-11-12T12:49:47Z

/retest

danielhelfand

Looks good otherwise, just a suggestion for the default plugin path.

danielhelfand · 2020-11-12T14:55:23Z

Suggested change

pluginDir = ".config/tkn/plugins"

pluginDir = "~/.config/tkn/plugins"

Oh right, I wanted to use homedir to expand this 😅

Updated, using homedir.Expand now 😉

@sthaha

This adds a basic execution plugin model, very simple to be honest. Most of the work has been done by @sthaha. In a gist this is how it works - If the specified subcommand doesn't exists, we check if a command with the subcommand prefixed by `tkn-` exists in the `$HOME/.config/tkn/pulgins` (for `foo` we look for `tkn-foo`). - The default folder (`$HOME/.config/tkn/plugins`) can be overriden with `TKN_PLUGINS_DIR`. - If the binary doesn't exists, we proceed as usual - If the binary exists, we execute it "à-la" `exec …`, aka we give full hand to the binary. ```bash $ ls ~/.config/tkn/plugins/tkn-* /home/vincent/.config/tkn/plugins/tkn-bar /home/vincent/.config/tkn/plugins/tkn-foo $ tkn foo is bar --hello args is bar --hello fooo bar $ tkn bar bar /usr/bin/env: ‘invalidcommand’: No such file or directory ``` Signed-off-by: Sunil Thaha <sthaha@redhat.com> Signed-off-by: Vincent Demeester <vdemeest@redhat.com>

vdemeester · 2020-11-13T08:44:23Z

#1252 fixes the cross error

vdemeester · 2020-11-13T16:02:24Z

/retest

vdemeester · 2020-11-13T16:25:15Z

/retest

chmouel · 2020-11-17T13:14:08Z

Is ~/.config/tkn the right place from a LSB distro/point of view ? it should be ~/.local/share isn't it ? But most people would be confused and would not care about FHS or LSB i guess, can we have ~/.config/tekton/plugins tho ? in case we end up with other stuff we want that is tekton related but not tkn releated.

krew has it in kube/plugins

vdemeester · 2020-11-17T13:43:40Z

Is ~/.config/tkn the right place from a LSB distro/point of view ? it should be ~/.local/share isn't it ? But most people would be confused and would not care about FHS or LSB i guess, can we have ~/.config/tekton/plugins tho ? in case we end up with other stuff we want that is tekton related but not tkn releated.

krew has it in kube/plugins

krew has .krew/bin doesn't it ? I choosed tkn instead of tekton, mainly because the tool is called tkn 😅. If there is another cli tool for tekton apis named foo in the future, what make tkn more inclined to use tekton instead of foo ?

I don't have strong opinion on this though, I just want us to go with a given setup and see how it goes and how it is useful.

vdemeester · 2020-12-01T09:15:37Z

@tektoncd/cli-maintainers are we good to merge given the result of the poll ?

tekton-robot · 2020-12-01T09:29:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danielhelfand, piyush-garg

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [danielhelfand,piyush-garg]

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

chmouel · 2020-12-01T12:11:39Z

/lgtm

Looking Good! 🤙🏽

#1260 | [vinamra28] Use goreleaser task from tektoncd/catalog | 2020/11/18-09:01 #1261 | [Pradeep Kumar] update README to v0.14.0 | 2020/11/18-12:25 #1259 | [Pratik Jagrut] Add --no-headers flag to tkn condition list command | 2020/11/25-12:19 #1257 | [Chmouel Boudjnah] Use debian/VERSION when building debian package | 2020/11/25-12:33 #1254 | [Pratik Jagrut] Add --no-headers flag to tkn triggerbinding list command | 2020/11/25-14:13 #1255 | [Pratik Jagrut] Add --no-headers flag to tkn triggertemplate list command | 2020/11/25-14:13 #1258 | [Pratik Jagrut] Add --no-headers flag to tkn eventlistener list command | 2020/11/25-15:11 #1178 | [Vincent Demeester] Basic execution plugin module 📟 | 2020/12/01-12:25 #1242 | [Daniel Helfand] allow --prefix-name and --timeout to be overrode when using --last or --use options | 2020/12/01-12:25 #1250 | [Sunghoon Kang] Ignore --follow flag when TaskRun or PipelineRun is done | 2020/12/02-15:28 null | [SM43] Bumps Hub CLI Dependency | 2020/12/16-12:32 null | [Matt Moore] Hoist an ActivityTimeout option. | 2020/12/16-18:26 null | [Pradeep Kumar] bump pipeline and triggers dep | 2020/12/17-07:47 null | [Daniel Helfand] remove --check shorthand and add global flags to version cmd | 2020/12/17-08:59 Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>

tekton-robot requested a review from a team September 18, 2020 14:56

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 18, 2020

vdemeester mentioned this pull request Sep 18, 2020

WIP: Attempt at adding a plugin sub-cmd #1136

Closed

vdemeester force-pushed the plugin-support branch from 3b3f233 to 33ecebd Compare September 18, 2020 14:58

tekton-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 18, 2020

vdemeester changed the title ~~Attempt at adding a plugin sub-cmd~~ Basic execution plugin module Sep 18, 2020

vdemeester force-pushed the plugin-support branch from 33ecebd to f68024e Compare September 18, 2020 14:59

sthaha reviewed Sep 20, 2020

View reviewed changes

Comment thread cmd/tkn/main.go Outdated

Copy link
Copy Markdown

Member

sthaha Sep 20, 2020

Choose a reason for hiding this comment

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

How about fmt.Fprintf(os.Stderr, ...

vdemeester reacted with thumbs up emoji

sthaha reviewed Sep 20, 2020

View reviewed changes

sm43 reviewed Sep 21, 2020

View reviewed changes

Comment thread cmd/tkn/main.go Outdated

sm43 reviewed Sep 21, 2020

View reviewed changes

vdemeester force-pushed the plugin-support branch 2 times, most recently from c846340 to e2b9661 Compare October 30, 2020 10:55

tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 30, 2020

vdemeester force-pushed the plugin-support branch from e2b9661 to 9c192b5 Compare October 30, 2020 11:03

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 30, 2020

sthaha reviewed Nov 1, 2020

View reviewed changes

sthaha reviewed Nov 2, 2020

View reviewed changes

tekton-robot assigned sthaha Nov 2, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 2, 2020

vdemeester force-pushed the plugin-support branch 2 times, most recently from 950c1ca to 32467f7 Compare November 10, 2020 15:09

vdemeester force-pushed the plugin-support branch 2 times, most recently from d30ec05 to 6a65632 Compare November 12, 2020 12:06

danielhelfand reviewed Nov 12, 2020

View reviewed changes

vdemeester force-pushed the plugin-support branch from 6a65632 to f8965e4 Compare November 13, 2020 08:14

vdemeester force-pushed the plugin-support branch from f8965e4 to 9ac3fb0 Compare November 13, 2020 08:22

danielhelfand approved these changes Nov 17, 2020

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 17, 2020

piyush-garg approved these changes Dec 1, 2020

View reviewed changes

tekton-robot assigned chmouel Dec 1, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 1, 2020

tekton-robot merged commit 2473842 into tektoncd:master Dec 1, 2020

vdemeester deleted the plugin-support branch December 1, 2020 12:31

	pluginDir = ".config/tkn/plugins"
	pluginDir = "~/.config/tkn/plugins"

Conversation

vdemeester commented Sep 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Submitter Checklist

Release Notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdemeester commented Oct 30, 2020

Uh oh!

vdemeester commented Oct 30, 2020

Uh oh!

vdemeester commented Oct 30, 2020

Uh oh!

sthaha left a comment

Choose a reason for hiding this comment

Uh oh!

vdemeester commented Nov 2, 2020

Uh oh!

sthaha left a comment

Choose a reason for hiding this comment

Uh oh!

danielhelfand commented Nov 10, 2020

Uh oh!

pradeepitm12 commented Nov 10, 2020

Uh oh!

danielhelfand commented Nov 11, 2020

Uh oh!

pradeepitm12 commented Nov 11, 2020

Uh oh!

danielhelfand commented Nov 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vdemeester commented Nov 12, 2020

Uh oh!

danielhelfand left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdemeester commented Nov 13, 2020

Uh oh!

vdemeester commented Nov 13, 2020

Uh oh!

vdemeester commented Nov 13, 2020

Uh oh!

chmouel commented Nov 17, 2020

Uh oh!

vdemeester commented Nov 17, 2020

Uh oh!

vdemeester commented Dec 1, 2020

Uh oh!

tekton-robot commented Dec 1, 2020

Uh oh!

chmouel commented Dec 1, 2020

Uh oh!

Reviewers

Assignees

vdemeester commented Sep 18, 2020 •

edited

Loading

danielhelfand commented Nov 11, 2020 •

edited

Loading