-
Notifications
You must be signed in to change notification settings - Fork 292
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
Add resource version for status reporting and update configuration types #1004
Conversation
60d2721
to
5c3f787
Compare
5c3f787
to
9ae1042
Compare
commGroupFieldKey = "commGroup" | ||
healthEndpointName = "/healthz" | ||
printAPIKeyCharCount = 3 | ||
configUpdaterInterval = 15 * time.Second |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will make this configurable in one of later PRs.
@@ -326,23 +326,22 @@ sources: | |||
my-annotation: "true" | |||
labels: | |||
my-label: "true" | |||
plugins: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was invalid nesting when marshalling config, which was fixed in this PR 🙂
I see 2 issues:
Both these issues stem from the push semantics of distributing resource version. |
The idea is that Config Updater will update the resource version. See this snippet ( |
That's every |
Not really, we should just report error and let user try again do that operation (e.g. edit SourceBindings or disable notifications). We should retry only if this is something only Deployment should do (e.g. update status, like in this case). |
5960fb8
to
675693c
Compare
@josefkarasek retry added - you can test it by starting the backend server, running Botkube to fetch the initial configuration and then quickly close it 😄 Or wait for startup reporting and then close Botkube to report shutdown. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm 👍
Description
Changes proposed in this pull request:
Testing
Run with remote config: