-
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
Refactor IBM MQ scaler and remove and deprecate variables #6034
Conversation
a1db827
to
3e31a12
Compare
Hello Could you update the PR to align it with our deprecation policy? I think that unifying is always better |
82f5b89
to
995b612
Compare
I think that is possible. I will work on it and get back to you as soon as possible. |
9e79292
to
a3e7f0d
Compare
Thanks. You are absolutely right! I have updated the PR. |
a61368f
to
c1d94ce
Compare
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 the changes for IBM scaler look really good!
ptal @JorTurFer
config/crd/bases/eventing.keda.sh_clustercloudeventsources.yaml
Outdated
Show resolved
Hide resolved
d8611d8
to
cc3aae8
Compare
ptal @kedacore/keda-core-contributors, I think this should be good to merge |
I forgot the |
LGTM, just an small nit |
Signed-off-by: Rick Brouwer <[email protected]>
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! ❤️
/run-e2e ibm |
) Signed-off-by: Rick Brouwer <[email protected]> Signed-off-by: Fira Curie <[email protected]>
) Signed-off-by: Rick Brouwer <[email protected]> Signed-off-by: Jorge Turrado <[email protected]>
queueManager
parameter is currently checked to see if it's filled, but it's not used elsewhere in the code. This unused code is removed.tlsDisabled
(as mentioned in the documentation) /tls
(as mentioned in the code) will be announced as being deprecated in a further release. All other scalers use the parameterunsafeSsl
. To maintain consistency between other scalers, we are making changes, asunsafeSsl
already handles this functionality.tls
will still be supported. When both variables are filled in, an error message is logged and returned. If onlytls
is filled in, its value will be assigned tounsafeSsl
. A deprecation info message will be logged. My proposal is to tag thetls
parameter withdeprecated=use unsafeSsl instead
in version 2.18 and remove the deprecated code.Checklist
Fixes #6033
Docs: 1445
Related: 5797