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

Send the cluster name for all interactive commands #1091

Merged
merged 1 commit into from
Jun 15, 2023

Conversation

mszostok
Copy link
Contributor

@mszostok mszostok commented Jun 14, 2023

Description

Changes proposed in this pull request:

-Send the cluster name for all interactive commands

The current implementation is quite hacky. The main problem is that only buttons are easily to handle as we know the whole command up front and we can simply add --cluster-name flag.

For all others, like selects, inputs, etc. the command is constructed either via

func resolveBlockActionCommand(act slack.BlockAction) (string, command.Origin) {
cmd := act.Value
cmdOrigin := command.UnknownOrigin
switch act.Type {
// currently we support only interactive.MultiSelect option
case "multi_static_select":
var items []string
for _, item := range act.SelectedOptions {
items = append(items, item.Value)
}
cmd = fmt.Sprintf("%s %s", act.ActionID, strings.Join(items, ","))
cmdOrigin = command.MultiSelectValueChangeOrigin
case "static_select":
// Example of commands that are handled here:
// @Botkube kcc --verbs get
// @Botkube kcc --resource-type
cmd = fmt.Sprintf("%s %s", act.ActionID, act.SelectedOption.Value)
cmdOrigin = command.SelectValueChangeOrigin
case "button":
cmdOrigin = command.ButtonClickOrigin
case "plain_text_input":
cmd = fmt.Sprintf("%s%q", act.BlockID, strings.TrimSpace(act.Value))
cmdOrigin = command.PlainTextInputOrigin
}
return cmd, cmdOrigin
}

or by using state

func (e *Kubectl) extractStateDetails(state *slack.BlockActionStates) stateDetails {
.

This introduces additional complexity that we need to handle when we are injecting the --cluster-name flag as otherwise we will break the parsing on the plugin side.

Notes

I tried to use a new field, use ids, and slack message metadata, none of them worked for me 😞 So I ended up with this kinda ugly solution.

Future

Because of the current architecture, I cannot normalize it in a more straightforward way. IMO we should decouple slack commands and slack state. I would see sth like:

  • clean cmd that is always passed to the plugin
  • optional metadata which is map[string]string that holds the data that we extract from the Slack state. This way our plugins won't parse the initial slack state and as a result we can hold more details in the state that will be only handled in the core code.

Testing

  • Install and play with the interactive kubectl or help message.

@mszostok mszostok added the enhancement New feature or request label Jun 14, 2023
@mszostok mszostok requested review from a team and PrasadG193 as code owners June 14, 2023 15:20
Copy link

@josefkarasek josefkarasek left a comment

Choose a reason for hiding this comment

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

lgtm 👍

@mszostok mszostok enabled auto-merge (squash) June 15, 2023 09:02
@mszostok mszostok merged commit a789ac3 into kubeshop:main Jun 15, 2023
@mszostok mszostok deleted the slack-app/cluter-name branch June 15, 2023 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants