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

Hearbeat reporting #1469

Merged

Conversation

madebyrogal
Copy link
Contributor

@madebyrogal madebyrogal commented Jul 4, 2024

MOD extend bots and sinks with errorMsg

Description

Changes proposed in this pull request:

  • Extend heart beat by adding health status report

Testing

Related issue(s)

https://github.com/kubeshop/botkube-cloud/issues/142
https://github.com/kubeshop/botkube-cloud/pull/1189

@madebyrogal madebyrogal requested review from a team and mszostok July 4, 2024 15:35
@madebyrogal madebyrogal marked this pull request as draft July 4, 2024 15:35
@madebyrogal madebyrogal added the enhancement New feature or request label Jul 4, 2024
@madebyrogal madebyrogal marked this pull request as ready for review July 8, 2024 05:44
Copy link
Collaborator

@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 e2e, works as expected 👌

But let's agree on code changes before merging 🙂

internal/heartbeat/noop_reporter.go Outdated Show resolved Hide resolved
@@ -61,6 +61,8 @@ const (
reportHeartbeatMaxRetries = 30
)

var healthNotifiers = make(map[string]health.Notifier)
Copy link
Collaborator

@mszostok mszostok Jul 8, 2024

Choose a reason for hiding this comment

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

  1. it doesn't make sens to have a global var here. Let's move it to the healtChecker that is responsible for it
  2. no need to repeat the interfaces (HealthNotifierSink/HealthNotifierBot). You should have a single interface in place which expect it (healthChecker)
  3. if / else statement is repeated, while it's common for all integrations, please see linked code changes to see how it can be changed.
  4. FailedSink and FailedBot, referring to the same. The healthChecker - it doesn't care, just have single one.
  5. You don't need to be worried about integration name, you can use it on 'nil'
     func Test(t *testing.T) {
        app, err := NewSocketSlackDemo()
        assert.Error(t, err)
        
        fmt.Println(app.IntegrationName())
     }
     
     func NewSocketSlackDemo() (*SocketSlack, error) {
        return nil, errors.New("fake")
     }

Changes in code based on feedback above: c472d4d

@madebyrogal madebyrogal force-pushed the implement-heartbeat-reporting branch from dea8ebe to 8466704 Compare July 9, 2024 08:26
@madebyrogal madebyrogal merged commit 70803a4 into kubeshop:main Jul 9, 2024
16 checks passed
@madebyrogal madebyrogal deleted the implement-heartbeat-reporting branch July 9, 2024 11:27
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