Skip to content
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

fix(kafka): Make parsing partitionLimitation more resilient against ws #4333

Merged
merged 1 commit into from
Mar 8, 2023

Conversation

AndreasBergmeier6176
Copy link
Contributor

Ignores whitespaces in partitionLimitation.
Fix a spelling mistake on the way.
Add a few more tests cases.
Note that currently it is not possible to configure a limitation so that
no partition will be consumed!

Checklist

Relates to #4328

@AndreasBergmeier6176 AndreasBergmeier6176 requested a review from a team as a code owner March 8, 2023 06:52
@JorTurFer
Copy link
Member

JorTurFer commented Mar 8, 2023

Thanks for the contribution!
After adding some test cases, other test (TestKafkaGetMetricSpecForScaling) is failing because it uses the data by position. Could you update it too? Basically you will need to adjust these indexes https://github.com/AndreasBergmeier6176/keda/blob/resilient_pl/pkg/scalers/kafka_scaler_test.go#L175-L179 (I think that changing 8 with 10 will be enough)

Ignores whitespaces in partitionLimitation.
Fix a spelling mistake on the way.
Add a few more tests cases.
Note that currently it is not possible to configure a limitation so that
no partition will be consumed!

Signed-off-by: Andreas Bergmeier <[email protected]>
@AndreasBergmeier6176
Copy link
Contributor Author

Thanks for the contribution! After adding some test cases, other test (TestKafkaGetMetricSpecForScaling) is failing because it uses the data by position. Could you update it too? Basically you will need to adjust these indexes https://github.com/AndreasBergmeier6176/keda/blob/resilient_pl/pkg/scalers/kafka_scaler_test.go#L175-L179 (I think that changing 8 with 10 will be enough)

Ah thx. Totally missed that.

@JorTurFer JorTurFer enabled auto-merge (squash) March 8, 2023 11:20
@JorTurFer
Copy link
Member

JorTurFer commented Mar 8, 2023

/run-e2e kafka*
Update: You can check the progress here

@zroubalik
Copy link
Member

@JorTurFer we forgot to add this to changelog :)

@AndreasBergmeier6176
Copy link
Contributor Author

@JorTurFer we forgot to add this to changelog :)

Would be more noise then information IMO.

@JorTurFer JorTurFer mentioned this pull request Mar 8, 2023
1 task
@JorTurFer
Copy link
Member

@JorTurFer we forgot to add this to changelog :)

There are days when it's better to not review/merge anything... Nice catch @zroubalik !

The missing changelog update: #4335

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants