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

Decrease the unmarshall error severity from err to warn #629

Merged
merged 1 commit into from Jul 4, 2022

Conversation

mszostok
Copy link
Contributor

@mszostok mszostok commented Jul 4, 2022

SUMMARY

This PR only decreases the unmarshall error severity from err to warn. To make it a bit better, we can contribute to the slack lib and replace:

err := fmt.Errorf("RTM Error: Received unmapped event %q", typeStr)

(source)

with error type UnmappedError. This won't be a breaking change for them, so we should be able to merge that.
As a result, we will know the details about the error and could implement sth like:

// handlingEvents holds event names which have corresponding handling logic.
var handlingEvents = sets.NewString("message", "connected")

// trimmed

      case *slack.UnmarshallingErrorEvent:
        err, ok := ev.ErrorObj.(slack.UnmappedError)
        if ok && !handlingEvents.Has(err.EventType()) { // it's not an err for us, as we don't care about this event type.
          b.log.Debugf("Slack unmarshalling error: %+v", ev.Error())
          continue
        }

        b.log.Errorf("Slack unmarshalling error: %+v", ev.Error())

In that way even if someone will configure custom Slack bot with wrong event subscription we will be able to “ignore them”.

WDYT? Is it worth it? If we stay with RTM, then it's the only option.

FINDINGS

I wanted to describe the workaround for this issue by creating the own Bot. I found that the current Bot is based on RTM. This means that it issues the old API.
Screen Shot 2022-07-04 at 12 36 47

In this case, we are not able to:

  • reduce the app permission
  • select events to which we want to be subscribed

The best option would be changing the implementation from RTM to WebSocket mode. Should I create an issue?

Fixes #619

@mszostok mszostok added this to To review in v0.13.0 Jul 4, 2022
@pkosiec
Copy link
Member

pkosiec commented Jul 4, 2022

The best option would be changing the implementation from RTM to WebSocket mode. Should I create an issue?

Can we switch to WebSocket mode? Is everything what we use supported (I mean in the Go Slack library)?
If yes, please create issue 👍 And then I would skip the contribution when it comes to the RTM method.

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.

I also thought about channging the default log level to make sure it is not seen by default (https://github.com/infracloudio/botkube/blob/develop/helm/botkube/values.yaml#L48), but I'd rather keep it as it is, so LGTM 👍

@mszostok
Copy link
Contributor Author

mszostok commented Jul 4, 2022

Issue created: #631 👍

@mergify mergify bot merged commit 77db4fa into kubeshop:develop Jul 4, 2022
v0.13.0 automation moved this from To review to Done Jul 4, 2022
@mszostok mszostok deleted the change-logging branch July 4, 2022 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Change Slack unmarshalling error from error to warning level
2 participants