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

Unify commands #867

Merged
merged 3 commits into from
Dec 13, 2022
Merged

Unify commands #867

merged 3 commits into from
Dec 13, 2022

Conversation

josefkarasek
Copy link

@josefkarasek josefkarasek commented Nov 28, 2022

Unify BotKube command names

Changes proposed in this pull request:

Related issue(s)

#703

@pkosiec
Copy link
Member

pkosiec commented Nov 28, 2022

Here's what I quickly drafted:

package execute

import (
	"context"
	"github.com/kubeshop/botkube/pkg/bot/interactive"
	"github.com/kubeshop/botkube/pkg/config"
)

//
// API
//

var noResourceNames = []string{""} // for resourceless executor

type CommandVerb string

const (
	ListCommandVerb    = "list"
	EnableCommandVerb  = "enable"
	DisableCommandVerb = "disable"
	PingCommandVerb    = "ping"
        // (...)
)

type CommandExecutor interface {
	Commands() map[CommandVerb]CommandFn
	ResourceNames() []string
}
type CommandFn func(ctx context.Context, cmdCtx CommandContext) (interactive.Message, error)

type CommandContext struct {
	Args            []string
	ClusterName     string
	CommGroupName   string
	BotName         string
	Conversation    Conversation
	Platform        config.CommPlatformIntegration
	notifierHandler NotifierHandler // for notification executor
	// etc.
}

//
// Example for resourceful executor
//

type actionExecutor struct{}

func (e *actionExecutor) ResourceNames() []string {
	return noResourceNames
}

func (e *actionExecutor) Commands() map[CommandVerb]CommandFn {
	return map[CommandVerb]CommandFn{
		ListCommandVerb:    e.List,
		EnableCommandVerb:  e.Enable,
		DisableCommandVerb: e.Disable,
	}
}

func (e *actionExecutor) List(ctx context.Context, cmdCtx CommandContext) (interactive.Message, error) {
	// do something
	panic("not implemented")
}
func (e *actionExecutor) Enable(ctx context.Context, cmdCtx CommandContext) (interactive.Message, error) {
	// do something
	panic("not implemented")
}
func (e *actionExecutor) Disable(ctx context.Context, cmdCtx CommandContext) (interactive.Message, error) {
	// do something
	panic("not implemented")
}

//
// example for Resourceless Executor
//

type pingExecutor struct{}

func (e *actionExecutor) ResourceNames() []string {
	return noResourceNames
}

func (e *pingExecutor) Commands() map[CommandVerb]CommandFn {
	return map[CommandVerb]CommandFn{
		PingCommandVerb: e.Ping,
	}
}

func (e *pingExecutor) Ping(ctx context.Context, cmdCtx CommandContext) (interactive.Message, error) {
	// do something
	panic("not implemented")
}

//
// Default executor
//

type SampleDefaultExecutor struct {
	cmdsMapping map[CommandVerb]map[string]CommandFn
}

// In main.go, call this constructor - initialize and pass all sub-executors to it
func NewSampleDefaultExecutor(executors []CommandExecutor) (*SampleDefaultExecutor, error) {
	cmdsMapping := make(map[CommandVerb]map[string]CommandFn)
	for _, executor := range executors {
		cmds := executor.Commands()
		resNames := executor.ResourceNames()

		for verb, cmdFn := range cmds {
			for _, resName := range resNames {
                               // TODO: Handle conflicts - return error from the constructor
				cmdsMapping[verb][resName] = cmdFn
			}
		}
	}

	return &SampleDefaultExecutor{cmdsMapping: cmdsMapping}, nil
}

func (s *SampleDefaultExecutor) Execute(ctx context.Context) interactive.Message {
	if len(args) == 0 {
		// invalid command, do something - ideally rework default executor to return error
	}

	cmdVerb := args[0]
	var cmdRes string
	if len(args) > 1 {
		cmdRes = args[1]
	}

	cmdCtx := CommandContext{
		// define necessary fields
	}
	res, err := s.cmdsMapping[cmdVerb][cmdRes](ctx, cmdCtx)
	// do something with res, err, first check if it exists
}

And here are some general comments:

  1. Please avoid using init to make code more readable and predictable. This is something we got rid of over last few months as it's hard to see the flow of data.
  2. Please extract all executors to separate types (in separate files) as the example shows, including kubectl one as well.
  3. (Stretch) If that possible, please refactor the DefaultExecutor to also return error, which could be handled inside every bot (the error can be logged there).
    Thanks!

@@ -220,7 +223,9 @@ func (e *DefaultExecutor) Execute(ctx context.Context) interactive.Message {
cmdVerb := strings.ToLower(args[0])
var cmdRes string
if len(args) > 1 {
cmdRes = strings.ToLower(args[1])
if !strings.Contains(args[1], "--cluster-name=") {
Copy link
Author

Choose a reason for hiding this comment

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

@mszostok any tips on how to support parameters like here?
@botkube help - args[0]="help", args[1]=""
@botkube help --cluster-name=c1 - args[0]="help", args[1]!=""

Copy link
Contributor

Choose a reason for hiding this comment

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

yep, this is tricky. In general, we should get rid of the--cluster-name flag once we will remove the legacy Slack: #865

However, for now we need to handle that. What's more, you need to handle:

  • @botkube help --cluster-name=foo
  • @botkube help --cluster-name foo // no = sign
  • @botkube kc get po --cluster-name foo -n default // whitespace, additional flag after

Currently, we have 2 approaches in our code base:

  1. Old approach still used in kubectl executor, where the --cluster-name is removed in the for loop:
    // TODO: This code was moved from:
    //
    // https://github.com/kubeshop/botkube/blob/0b99ac480c8e7e93ce721b345ffc54d89019a812/pkg/execute/executor.go#L242-L276
    //
    // Further refactoring in needed. For example, the cluster flag should be removed by an upper layer
    // as it's strictly Botkube related and not executor specific (e.g. kubectl, helm, istio etc.).
    func (e *Kubectl) getFinalArgs(args []string) []string {
    // Remove unnecessary flags
    var finalArgs []string
    isClusterNameArg := false
    for _, arg := range args {
    if isClusterNameArg {
    isClusterNameArg = false
    continue
    }
    if arg == AbbrFollowFlag.String() || strings.HasPrefix(arg, FollowFlag.String()) {
    continue
    }
    if arg == AbbrWatchFlag.String() || strings.HasPrefix(arg, WatchFlag.String()) {
    continue
    }
    // Remove --cluster-name flag and it's value
    if strings.HasPrefix(arg, ClusterFlag.String()) {
    // Check if flag value in current or next argument and compare with config.settings.clusterName
    if arg == ClusterFlag.String() {
    isClusterNameArg = true
    }
    continue
    }
    finalArgs = append(finalArgs, arg)
    }
    return finalArgs
    }
  2. New approach which is more reliable where cobra flag parser is used and later flag is removed from a given string:
    func extractExecutorFilter(cmd string) (executorFilter, error) {
    var filters []string
    filters, err := parseAndValidateAnyFilters(cmd)
    if err != nil {
    return nil, err
    }
    if len(filters) == 0 {
    return newExecutorEchoFilter(cmd), nil
    }
    if len(filters[0]) == 0 {
    return nil, errors.New(missingCmdFilterValue)
    }
    filterVal := filters[0]
    escapedFilterVal := regexp.QuoteMeta(filterVal)
    filterFlagRegex, err := regexp.Compile(fmt.Sprintf(`--filter[=|(' ')]*('%s'|"%s"|%s)("|')*`,
    escapedFilterVal,
    escapedFilterVal,
    escapedFilterVal))
    if err != nil {
    return nil, errors.New("could not extract provided filter")
    }
    matches := filterFlagRegex.FindStringSubmatch(cmd)
    if len(matches) == 0 {
    return nil, fmt.Errorf(filterFlagParseErrorMsg, cmd, "it contains unsupported characters.")
    }
    return newExecutorTextFilter(filterVal, strings.ReplaceAll(cmd, fmt.Sprintf(" %s", matches[0]), "")), nil

IMO you can copy and adapt the 2nd approach for cluster-name and simply remove it from the command string.

Copy link
Member

Choose a reason for hiding this comment

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

@mszostok Just a minor comment about

In general, we should get rid of the--cluster-name flag once we will remove the legacy Slack

I don't think we can do this, if multi-cluster is also supported by Mattermost and Discord. Or, do you mean enforcing users to create a dedicated app per cluster for all comm platforms? If that's not a common use-case, then maybe it makes sense 🤔 to be discussed later, I put it in the #865

@mszostok
Copy link
Contributor

mszostok commented Dec 7, 2022

FYI: @josefkarasek to make the e2e test pass, you need to also adjust the command name in e2e test. For example here:

command := "filters list"

it shouldn't be filters list but list filters. Currently, you get Command not supported. Please use 'help' to see supported commands. response so our assertion fails.

@josefkarasek josefkarasek added enhancement New feature or request breaking Contains breaking change labels Dec 7, 2022
@josefkarasek josefkarasek marked this pull request as ready for review December 8, 2022 09:58
@josefkarasek josefkarasek requested review from a team and PrasadG193 as code owners December 8, 2022 09:58
pkg/execute/executor.go Outdated Show resolved Hide resolved
@josefkarasek josefkarasek force-pushed the unify-cmds branch 2 times, most recently from b41fe28 to f625bb3 Compare December 8, 2022 13:09
Copy link
Contributor

@mszostok mszostok left a comment

Choose a reason for hiding this comment

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

Overall good job 🚀

I reviewed the code and left some comments. I also tested that locally and found 3 issues:

@@ -47,6 +33,40 @@ const (
humanReadableCommandListName = "Available kubectl commands"

lineLimitToShowFilter = 16

helpMessageList = `@Botkube list [resource]
Copy link
Contributor

Choose a reason for hiding this comment

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

It's ok-ish approach for now. However, I would suggest adding TODO comment and addressing that in the follow-up PR where this will be automatically generated. Currently, aliases are defined in two different places and it's error-prone when we will add more or change existing one.

Copy link
Author

Choose a reason for hiding this comment

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

Yes. let's do it in a subsequent PR.


fn, found := resources[cmdRes]
if !found {
e.reportCommand(anonymizedInvalidVerb, false)
Copy link
Contributor

Choose a reason for hiding this comment

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

this is about verb, so you should report that in line 272.
Here you should anonymize only resource name, so do sth like:
e.reportCommand(fmt.Sprintf("%s {invalid resource}", cmdVerb), false)

Copy link
Author

@josefkarasek josefkarasek Dec 12, 2022

Choose a reason for hiding this comment

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

Good point - it was a mistake on my part. To be consistent with previous implementation, I moved it a few lines above to the CommandVerb parsing.

const enabled = true
cmdVerb, cmdRes := cmdCtx.Args[0], cmdCtx.Args[1]

defer e.reportCommand(cmdVerb, cmdRes, cmdCtx.Conversation.CommandOrigin, cmdCtx.Platform)
Copy link
Contributor

Choose a reason for hiding this comment

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

we need to be careful about reporting our commands. This data is used on our dashboards: https://kubeshop.slack.com/archives/C03MRCX7UE9/p1662727250564769

Please sync with @brampling and inform him about changes that we will introduce because of changed syntax, so it can be handled properly too.

Copy link
Author

@josefkarasek josefkarasek Dec 12, 2022

Choose a reason for hiding this comment

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

This is consistent with what we did before this PR. Obviously we should review it, just to be sure, but semantically there's no change.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not consistent as the new command is different, e.g. from notifier [start|stop|status] is now start notifications etc.

I posted it here, but it was meant to be a generic comment about changed syntax. Not necessary in the scope of "actions".

pkg/execute/action.go Outdated Show resolved Hide resolved
pkg/execute/executor.go Outdated Show resolved Hide resolved
pkg/execute/executor.go Outdated Show resolved Hide resolved
pkg/execute/filter.go Outdated Show resolved Hide resolved
@@ -16,12 +16,17 @@ import (
"github.com/kubeshop/botkube/pkg/sliceutil"
)

const (
// KubectlBinary is absolute path of kubectl binary
KubectlBinary = "/usr/local/bin/kubectl"
Copy link
Contributor

Choose a reason for hiding this comment

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

why it's exported?

Copy link
Author

Choose a reason for hiding this comment

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

I'm now getting the cluster version in main.go, similarly to the initial help message -

botkube/cmd/botkube/main.go

Lines 164 to 168 in bf454cb

k8sVersion, err := findK8sVersion(runner)
if err != nil {
return reportFatalError("while fetching kubernetes version", err)
}
botkubeVersion := findBotkubeVersion(k8sVersion)

pkg/execute/notifier.go Outdated Show resolved Hide resolved
helm/botkube/values.yaml Show resolved Hide resolved
@josefkarasek
Copy link
Author

Overall good job rocket

I reviewed the code and left some comments. I also tested that locally and found 3 issues:

  • @Botkube status sourcebindings - doesn't return expect response. Please see the Lacho comment to check desired output.
    Screen Shot 2022-12-09 at 15 13 09
  • @Botkube status - doesn't return expect response. Please see the Lacho comment to check desired output.
    Screen Shot 2022-12-09 at 15 13 09
  • filtering doesn't work globally. It's related to this comment: #867 (comment)
    Screen Shot 2022-12-09 at 15 13 09

Point 1) and 3) should be addressed by now. I'd prefer to push 2) to a subsequent PR, as it's related to the changes that need to happen to support better help messages, when the user misspell verb.

@Botkube status - print event notifier status AND display a message listing all status options (listed above)

Copy link
Contributor

@mszostok mszostok left a comment

Choose a reason for hiding this comment

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

Tested and it works awesome 🚀

Let's address last comments and we will be ready for merging this PR!


const redactedSecretStr = "*** REDACTED ***"

// Deprecated: this function doesn't fit in the scope of notifier. It was moved from legacy reasons, but it will be removed in future.
Copy link
Contributor

Choose a reason for hiding this comment

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

this comment should be removed as this func is not a part of notifier anymore 👍

const enabled = true
cmdVerb, cmdRes := cmdCtx.Args[0], cmdCtx.Args[1]

defer e.reportCommand(cmdVerb, cmdRes, cmdCtx.Conversation.CommandOrigin, cmdCtx.Platform)
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not consistent as the new command is different, e.g. from notifier [start|stop|status] is now start notifications etc.

I posted it here, but it was meant to be a generic comment about changed syntax. Not necessary in the scope of "actions".


fn, found := resources[cmdRes]
if !found {
e.log.Infof("received unsupported resource: %q", execFilter.FilteredCommand())
Copy link
Contributor

Choose a reason for hiding this comment

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

please also report that e.reportCommand(fmt.Sprintf("%s {invalid resource}", cmdVerb), false) as I mentioned here: #867 (comment)


// Config returns Config in yaml format
func (e *ConfigExecutor) Config(ctx context.Context, cmdCtx CommandContext) (interactive.Message, error) {
cmdVerb := cmdCtx.Args[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add len check 🙂


out, err := e.showControllerConfig()
if err != nil {
return interactive.Message{}, fmt.Errorf("while executing 'showconfig' command: %w", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return interactive.Message{}, fmt.Errorf("while executing 'showconfig' command: %w", err)
return interactive.Message{}, fmt.Errorf("while rendering Botkube configuration: %w", err)

cmdVerb := cmdCtx.Args[0]
defer e.reportCommand(cmdVerb, cmdCtx.Conversation.CommandOrigin, cmdCtx.Platform)

out, err := e.showControllerConfig()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
out, err := e.showControllerConfig()
out, err := e.renderBotkubeConfiguration()

the current name is not accurate

if err != nil {
return interactive.Message{}, fmt.Errorf("while executing 'showconfig' command: %w", err)
}
msg := fmt.Sprintf("Showing config for cluster %q:\n\n%s", cmdCtx.ClusterName, out)
Copy link
Contributor

Choose a reason for hiding this comment

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

We have the cluster name already in the command header. IMO, we can remove this message and return raw configuration only.

@@ -66,7 +67,9 @@ func newCmdsMapping(executors []CommandExecutor) map[CommandVerb]map[string]Comm
cmdsMapping[verb] = make(map[string]CommandFn)
}
for _, resName := range resNames {
// TODO: Handle conflicts - return error from the constructor
if _, ok := cmdsMapping[verb][resName]; ok {
panic(fmt.Sprintf("Command collision: tried to register '%s %s', but it already exists", verb, resName))
Copy link
Contributor

Choose a reason for hiding this comment

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

We cannot use panic in the production code. Please return an error.

h.btnBuilder.ForCommandWithoutDesc("Start notifications", "start notifications"),
h.btnBuilder.ForCommandWithoutDesc("Stop notifications", "stop notifications"),
h.btnBuilder.ForCommandWithoutDesc("Get status", "status notifications"),
h.btnBuilder.ForCommandWithoutDesc("Display configuration", "config"),
Copy link
Contributor

Choose a reason for hiding this comment

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

The "Display configuration" button is misleading. It's under the notification section but actually it is about the whole Botkube configuration. For now, let's remove it from this section and discuss that async with @lpetkov and introduce it in a follow-up PR if needed.

Screen Shot 2022-12-13 at 09 52 52

type NotifierAction string
Available resources:
actions | action | act list available automations
filters | filter | fil list available filters
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
filters | filter | fil list available filters
filters | filter | flr list available filters

First inconsistency 😄 so it proves that this comment is valid: #867 (comment) 😄

Josef Karasek added 3 commits December 13, 2022 10:45
`@Botkube [command/verb] [optional secondary command] [--option="string"]`

`@Botkube ping [--cluster="string"]` - Ping a cluster
`@Botkube help` - Print help menu
`@Botkube kc|kcc|kubectl [command] [TYPE] [NAME] [flags]` - Run kubectl commands
`@Botkube config` - Show bot configuration

`@Botkube status [notifications|notif]`  - Show running status of event notifier
`@Botkube status [k|kc|kcc|kubectl]`  - Show enabled commands/verbs/resource types for `kubectl` for the channel
`@Botkube status sourcebindings` - Show enabled source bindings for the channel

`@Botkube list [filters|filter|flr]` to list all filters
`@Botkube list [commands|cmd]` to list all commands
`@Botkube list` - list all `list` options

`@Botkube enable [notifications|notif]` to enable notifications in a channel
`@Botkube enable [filters|filter|flr] filterName` to enable a filter
`@Botkube enable` - list all `enable` options

`@Botkube disable [notifications|notif]` to disable notifications in a channel
`@Botkube disable [filters|filter|flr] filterName` to disable a filter
`@Botkube disable` - list all `disable` options

`@Botkube edit sourcebindings`
`@Botkube edit` - list all `edit` options
Copy link
Contributor

@mszostok mszostok left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@josefkarasek josefkarasek merged commit c549e31 into kubeshop:main Dec 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Contains breaking change enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants