-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Feature/splunk observability scaler #6192
base: main
Are you sure you want to change the base?
Feature/splunk observability scaler #6192
Conversation
… in hindsight Signed-off-by: Sebastian Schimper <sschimper@splunk.com>
Signed-off-by: Sebastian Schimper <sschimper@splunk.com>
tests/scalers/splunk_observability_test/splunk_observability_test.go
Outdated
Show resolved
Hide resolved
Signed-off-by: Sebastian Schimper <sschimper@splunk.com>
The only files we should be changing under |
name: splunk-secrets | ||
namespace: {{.TestNamespace}} | ||
data: | ||
accessToken: YW1JeUpqVHRJd185cDhOWG01X21KQQ== # one time through-away access token used just for testing |
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.
It seems this test relies on an actual upstream being available to communicate with. Would it be possible to create a pod here that simply mocks responses? We could override the endpoint in the scaler config and point it to our mocked API. This would ensure the tests could still run in a more closed loop fashion without any upstream dependencies. Thoughts?
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.
agree
kedautil "github.com/kedacore/keda/v2/pkg/util" | ||
) | ||
|
||
type splunkObservabilityMetadata struct { |
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.
Should we support a parameter to override an endpoint in case that changes in the future?
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.
Note to self: TODO base url overwriting
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.
do we expect the endpoint to be changed? if so we can add the parameter.
tests/scalers/splunk_observability_test/splunk_observability_test.go
Outdated
Show resolved
Hide resolved
We should probably set this PR to WIP/Draft state |
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.
Thanks for the contribution! could of points
- helm update is not needed
- please fix DCO
@@ -16,7 +16,7 @@ | |||
// Code generated by protoc-gen-go. DO NOT EDIT. | |||
// versions: | |||
// protoc-gen-go v1.34.2 | |||
// protoc v5.27.3 | |||
// protoc v4.25.2 |
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.
please don't include these changes in the pb files, not related
kedautil "github.com/kedacore/keda/v2/pkg/util" | ||
) | ||
|
||
type splunkObservabilityMetadata struct { |
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.
do we expect the endpoint to be changed? if so we can add the parameter.
name: splunk-secrets | ||
namespace: {{.TestNamespace}} | ||
data: | ||
accessToken: YW1JeUpqVHRJd185cDhOWG01X21KQQ== # one time through-away access token used just for testing |
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.
agree
Thank you for reviewing, @zroubalik. I will work during the next days on fixing the things pointed out. |
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.
Thank you for reviewing, @zroubalik. I will work during the next days on fixing the things pointed out.
@sschimper-splunk any updates here please? We will cut a new release Nov 7, so if you want this included in KEDA 2.16 we should resolve this asap. If you don't have we can release this in the following release, not a problem :)
Hi @zroubalik, |
With this pull request, I would like to add a new custom KEDA scaler that interacts with the Splunk Observability Cloud Platform. It is able to query metrics from Splunk Observability Cloud and scale a deployment according to a predefined target value.
As for now, I do not have the created a pull request to update the Helm chart, becasue I did not think it necessary. However, my knowledge about Helm charts is admittedly limited, and I am happy to fix this in hindsight if that is necessary.
Thank you.
Checklist
Relates to: