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

Support Help() in plugins #948

Merged
merged 15 commits into from
Jan 31, 2023
Merged

Support Help() in plugins #948

merged 15 commits into from
Jan 31, 2023

Conversation

josefkarasek
Copy link

@josefkarasek josefkarasek commented Jan 19, 2023 •
edited

Provide an option to get plugin related help #893

Changes proposed in this pull request:

  • Add Help() function to the executor interface

Test

  1. Rebuild plugins and plugin index make gen-plugins-index
  2. Delete local plugin cache, for example rm -rf /tmp/plugin*
  3. Run local plugin server env PLUGIN_SERVER_HOST=http://localhost go run test/helpers/plugin_server.go
  4. Reference this plugin server in your config file
  5. Enable executors
executors:
  'echo':
    botkube/echo:
      enabled: true
  'helm':
    botkube/helm:
      enabled: true
  'gh':
    botkube/gh:
      enabled: true
  1. Run botkube locally: go run cmd/botkube/main.go
  2. See help output for plugins gh, helm, echo

Related issue(s)

Resolves #893

josefkarasek
josefkarasek commented Jan 19, 2023
View reviewed changes
internal/executor/helm/executor.go Outdated
@@ -131,6 +131,12 @@ func (e *Executor) Execute(ctx context.Context, in executor.ExecuteInput) (execu
}
}

func (Executor) Help() executor.HelpResponse {
return executor.HelpResponse{
Help: (&HelpCommand{}).Help(),
Copy link
Author

Choose a reason for hiding this comment

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

What should happen with existing Helm plugin help sub-command?

Copy link
Member

Choose a reason for hiding this comment

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

I think it should be totally overriden, so it shouldn't be possible to run such underlying sub-command. The plugin shouldn expose help only via gRPC API, and shouldn't handle {pluginName} help as user won't be able to execute it.

@josefkarasek josefkarasek marked this pull request as ready for review January 20, 2023 08:25
@josefkarasek josefkarasek requested review from a team and PrasadG193 as code owners January 20, 2023 08:25
@pkosiec pkosiec requested review from pkosiec and removed request for huseyinbabal January 27, 2023 08:43
@pkosiec pkosiec added the enhancement New feature or request label Jan 27, 2023
pkosiec
pkosiec reviewed Jan 27, 2023
View reviewed changes
Copy link
Member

@pkosiec pkosiec left a comment

Choose a reason for hiding this comment

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

Here are some comments from the code review. I'll test it after fixes 🙂

You can also provide some small instruction in the PR desc how did you test it (executor with help like helm, or the one with empty help, like echo).

pkg/api/executor/grpc_adapter.go Outdated Show resolved Hide resolved
pkg/api/executor/grpc_adapter.go Outdated Show resolved Hide resolved
pkg/api/plugin.go Outdated
Comment on lines 36 to 55
type HelpOutput struct {
Help string
}

Copy link
Member

Choose a reason for hiding this comment

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

Desn't seem to be used anywhere 🤔 If it is, then where and why the Help property is string?

internal/executor/helm/executor.go Outdated
@@ -131,6 +131,12 @@ func (e *Executor) Execute(ctx context.Context, in executor.ExecuteInput) (execu
}
}

func (Executor) Help() executor.HelpResponse {
return executor.HelpResponse{
Help: (&HelpCommand{}).Help(),
Copy link
Member

Choose a reason for hiding this comment

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

I think it should be totally overriden, so it shouldn't be possible to run such underlying sub-command. The plugin shouldn expose help only via gRPC API, and shouldn't handle {pluginName} help as user won't be able to execute it.

proto/executor.proto Outdated
@@ -33,7 +33,12 @@ message JSONSchema {
string refURL = 2;
Copy link
Member

Choose a reason for hiding this comment

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

This new API should be documented. Please open PR to Botkube docs and ensure the plugins docs (e.g. https://docs.botkube.io/plugin/custom-executor) are up to date.

internal/executor/helm/executor.go Show resolved Hide resolved
cmd/executor/echo/main.go Outdated Show resolved Hide resolved
internal/executor/helm/executor.go Outdated Show resolved Hide resolved
pkg/execute/executor.go Outdated
Comment on lines 172 to 174
if isHelpCmd(cmdCtx.Args) {
msg, err := e.pluginExecutor.Help(ctx, e.conversation.ExecutorBindings, cmdCtx.Args, cmdCtx.CleanCmd)
if err != nil {
e.log.Errorf("while executing help command %q: %s", cmdCtx.CleanCmd, err.Error())
return empty
}
return msg
}
Copy link
Member

Choose a reason for hiding this comment

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

  1. We should report the help command usage as well
  2. Once you add the command reporting, then I think the code can look even more complicated - consider extracting this help usage to a separate small method.

pkg/execute/plugin_executor.go Outdated
Comment on lines 159 to 164
func isHelpCmd(s []string) bool {
if len(s) < 2 {
return false
}
return s[1] == "help"
}
Copy link
Member

Choose a reason for hiding this comment

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

It is used only once, and only in the execute/executor.go, so please move the usage there.

@josefkarasek josefkarasek mentioned this pull request Jan 30, 2023
Merged
@josefkarasek josefkarasek added this to the v0.18.0 milestone Jan 30, 2023
pkosiec
pkosiec reviewed Jan 30, 2023
View reviewed changes
Copy link
Member

@pkosiec pkosiec left a comment

Choose a reason for hiding this comment

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

In general LGTM, just a final batch of small comments 👍

internal/executor/helm/executor.go Show resolved Hide resolved
cmd/executor/gh/main.go Outdated Show resolved Hide resolved
test/e2e/bots_test.go Show resolved Hide resolved
pkosiec
pkosiec approved these changes Jan 31, 2023
View reviewed changes
Copy link
Member

@pkosiec pkosiec left a comment

Choose a reason for hiding this comment

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

LGTM 👍 E2E tests are failing because of the formatting issue of the Helm help, but basically everything works well and all comments were addressed 👌 Thanks!

@josefkarasek josefkarasek merged commit 98965ac into kubeshop:main Jan 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pkosiec pkosiec pkosiec approved these changes

@PrasadG193 PrasadG193 Awaiting requested review from PrasadG193 PrasadG193 is a code owner

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
Botkube
Status: Done
Milestone
v0.18.0
Development

Successfully merging this pull request may close these issues.

Provide an option to get plugin related help
2 participants
@josefkarasek @pkosiec