-
Notifications
You must be signed in to change notification settings - Fork 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
Deprecation cleanup for 2.12 release #4806
Deprecation cleanup for 2.12 release #4806
Conversation
Thank you for your contribution! 🙏 We will review your PR as soon as possible.
While you are waiting, make sure to:
Learn more about: |
@JorTurFer I will take a look at the failed CI test. https://github.com/kedacore/keda/actions/runs/5600564910/jobs/10243134785?pr=4806. Seems like the removal of the CI breaks something 😆 |
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.
Looking good, just an small nit inline.
Could you update changelog adding this breaking change?
We should review docs as well to remove the parameter from both places at once
pkg/scalers/azure_blob_scaler.go
Outdated
meta.MetricName = kedautil.NormalizeString(fmt.Sprintf("azure-blob-%s", meta.BlobContainerName)) | ||
} | ||
|
||
meta.MetricName = kedautil.NormalizeString(fmt.Sprintf("azure-blob-%s", meta.BlobContainerName)) |
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 need to generate this here? I mean, if we have only one option for MetricName
and it's used once, maybe we could remove the object variable and generate the name wherever is used. WDYT? (If you agree, this also applies to other scalers)
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.
Yes. I just did another round of refactoring to remove this metricName
object var. Added a few test cases. Please help me review it 🙏
Could you resolve the conflicts with |
Thanks @JorTurFer 😄. I will resolve the conflct in |
Sure, no rush at all 😄 |
fa09231
to
6611c38
Compare
0d85b64
to
9bc2ec8
Compare
we should update this comment too: Lines 224 to 226 in b18ccd6
|
@JorTurFer wdyt about this ?
|
I'd just say something like:
It's something that never should happen thanks to the index in the prefix, and if it happens, we should review them in the code, so opening an issue is worth |
9bc2ec8
to
da4d346
Compare
{map[string]string{"metricName": "testName", "tenantIdFromEnv": "d248da64-0e1e-4f79-b8c6-72ab7aa055eb", "clientIdFromEnv": "41826dd4-9e0a-4357-a5bd-a88ad771ea7d", "clientSecretFromEnv": "U6DtAX5r6RPZxd~l12Ri3X8J9urt5Q-xs", "workspaceIdFromEnv": "074dd9f8-c368-4220-9400-acb6e80fc325", "query": query, "threshold": "1900000000"}, 1, "azure-log-analytics-testName"}, | ||
} | ||
|
||
func TestLogAnalyticsParseMetadataMetricName(t *testing.T) { |
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 remove this chunk because it is testing against an object variable metricName
which we don't use
@@ -251,7 +239,7 @@ func (s *prometheusScaler) Close(context.Context) error { | |||
} | |||
|
|||
func (s *prometheusScaler) GetMetricSpecForScaling(context.Context) []v2.MetricSpec { | |||
metricName := kedautil.NormalizeString(fmt.Sprintf("prometheus-%s", s.metadata.metricName)) |
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.
@JorTurFer Do you have any context why we have another extra prometheus
prefix here . I will just remove it. wdyt?
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
d6f5484
to
6af6b6a
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.
Thanks for this contribution! 💪
/run-e2e |
Signed-off-by: Yoon Park <yoongon.park@gmail.com>
Signed-off-by: anton.lysina <alysina@gmail.com>
Hi team,
I did a cleanup for 2.12 which is going to be released somewhere in Sept 2023. 🙏 . Its still a WIP and I will update the keda-docs repo later
Checklist
metricName
field in docs for 2.12 release keda-docs#1205Fixes: #4899