-
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
Use Operator SDK framework #387
Conversation
6149b84
to
8b62b50
Compare
Into PR's description I've added instructions on how to deploy this code. |
314fecc
to
0ce4182
Compare
0ce4182
to
472edaf
Compare
I've tweaked rest of the outstanding minor issues. I've tested this on OpenShift 4 using Kafka trigger, worked as expected. The code is ready for a Review and Merge. Helm chart update might be needed after merging this PR |
18f2056
to
e720356
Compare
@tomkerkhove you might be interested to take a look on this PR |
Thanks for the ping @zroubalik but I'll leave it up to @ahmelsayed as I'm not that knowledgeable on this level. |
I'll spend some time this week to test out this fork, @ahmelsayed plans to review sometime in the next week |
e720356
to
7afe733
Compare
Validated that this works with my demos around Service Bus and Rabbit MQ. Thanks @zroubalik |
|
||
COPY build/_output/bin/keda-adapter ${OPERATOR} | ||
|
||
COPY build/bin /usr/local/bin |
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 think this line is not needed, right? It fails for me on both images on a make build
. Removing it from both resolves the issue.
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.
Actually I think that folder is just missing the user_setup
script.
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.
Good catch, thanks. .gitignore
was too strict and prevented pushing those files. There's so much files, so I haven't noticed that. I'll fix it in a few.
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.
@ahmelsayed should be fixed now, PTAL
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 very much @zroubalik. The change looks good to me. instead of going over every file change here, I tested main scenarios and make sure I can read the new operator-sdk code.
make build
has that one issue about the missing bin folder, but it's a very minor thing. I'll merge it once you add that folder (or I can also just merge it and send a PR with it since I already need to update some of the CI scripts)
7afe733
to
5cd0ec2
Compare
@ahmelsayed @jeffhollan I have added one last minor change. The original cluster permissions were to wide (granting cluster-admin to the keda SA). I have restricted the permissions a little bit and have tested it and working for me. I might want to test it with your scenarios or at least one :) |
@@ -1,115 +1,74 @@ | |||
module github.com/kedacore/keda | |||
|
|||
go 1.13 | |||
go 1.13.1 |
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.
The go version is supposed to be "go ." this is producing that I'm not able to use this package as dependency for a controller.
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.
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 a lot!
Keda controller is rewritten to use operator-sdk framework. The controller logic was simplified removing informers, locks...
Usage changes
Major internal changes
sync.RWMutex
in ScaleHandler, etc)GetExternalMetricNames()
andGetScaledObjectMetrics()
are moved from ScaleHandler directly to the providerlog.Debug(msg)
are now logged under Level 1, ie.log.V(1).Info(msg)
, this allow us to add more granularity in logging in the future (introduce Level 2/3/4 if needed)Deploying this version of Keda
This implementation can be deployed by running these commands, it is expecting existing
keda
namespaceTODO
set appropriate loging levels(V(?)
for log messages previously marked asDebug
sections in the codeDebug
->V(1)
)tweak adapter loggingtest adapter(Kafka scaler working correctly)test namespaced vs cluster wide operator