-
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
New IBM MQ Scaler #1254
New IBM MQ Scaler #1254
Conversation
Add IBM Scaler
Add TLS option
Improve comments and clean up code
MQ Scaler Rebase
…to IBM-MQ-Scaler
Merge with v2
Merge with v2
Could you please fix the DCO? https://github.com/kedacore/keda/blob/v2/CONTRIBUTING.md#i-didnt-sign-my-commit-now-what (might be actually easier for you to squash all your commits into one and sign that one) There are some linting problems, see the Static Check Details. You can always check your code before commiting by running |
@@ -0,0 +1,218 @@ | |||
/** | |||
* © Copyright IBM Corporation 2020 |
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 IBM Copyright shouldn't be here.
@@ -0,0 +1,121 @@ | |||
/** | |||
* © Copyright IBM Corporation 2020 |
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.
dtto
|
||
resp, err := client.Do(req) | ||
if err != nil { | ||
panic(err) |
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 it should just return the error rather than panicing the entire process.
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 @ahmelsayed, we've included a potential fix for this in PR #1259
var response CommandResponse | ||
json.Unmarshal(body, &response) | ||
|
||
return response.CommandResponse[0].Parameters.Curdepth, nil |
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 might panic if CommandResponse
was empty, right?
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 @ahmelsayed, we've included a potential fix for this in PR #1259
} | ||
defer resp.Body.Close() | ||
|
||
body, _ := ioutil.ReadAll(resp.Body) |
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.
nit: handle ioutil read error in case the body stream got closed in the interim
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 @ahmelsayed, we've included a potential fix for this in PR #1259
body, _ := ioutil.ReadAll(resp.Body) | ||
|
||
var response CommandResponse | ||
json.Unmarshal(body, &response) |
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.
nit: handle Unmarshal
returned error in case body is not a valid JSON
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 @ahmelsayed, we've included a potential fix for this in PR #1259
targetQueueLengthQty := resource.NewQuantity(int64(s.metadata.targetQueueDepth), resource.DecimalSI) | ||
externalMetric := &v2beta2.ExternalMetricSource{ | ||
Metric: v2beta2.MetricIdentifier{ | ||
Name: kedautil.NormalizeString(fmt.Sprintf("%s-%s", "IBMMQ", s.metadata.queueName)), |
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 metric name should be unique, that means that if you specify two or more IBM MQ scalers in one ScaledObject, each scaler should have a different metric name. I wonder if this is enough?
Is viable a usecase, where we have two MQ scalers with the same queueName
but pointing to the different MQ (server)?
Thanks for all your feedback so far! 👍 |
Great thanks! I'm closing this, let's continue on the new PR |
As outlined in issue #1253:
Checklist