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

feat: add support for kafka event startingPositionTimestamp #11479

Merged

Conversation

overbit
Copy link
Contributor

@overbit overbit commented Oct 27, 2022

Closes: #11478

@codecov
Copy link

codecov bot commented Oct 27, 2022

Codecov Report

Base: 85.79% // Head: 85.66% // Decreases project coverage by -0.12% ⚠️

Coverage data is based on head (47654a3) compared to base (d2b6926).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11479      +/-   ##
==========================================
- Coverage   85.79%   85.66%   -0.13%     
==========================================
  Files         314      314              
  Lines       13259    13125     -134     
==========================================
- Hits        11375    11243     -132     
+ Misses       1884     1882       -2     
Impacted Files Coverage Δ
lib/plugins/aws/package/compile/events/kafka.js 98.73% <100.00%> (+0.08%) ⬆️
lib/plugins/aws/info/display.js 63.82% <0.00%> (-12.56%) ⬇️
lib/cli/interactive-setup/index.js 85.71% <0.00%> (-5.47%) ⬇️
lib/plugins/aws/deploy/lib/upload-artifacts.js 90.90% <0.00%> (-3.41%) ⬇️
lib/cli/interactive-setup/deploy.js 89.47% <0.00%> (-2.42%) ⬇️
commands/login.js 94.73% <0.00%> (-1.10%) ⬇️
lib/classes/service.js 84.13% <0.00%> (-0.48%) ⬇️
lib/serverless.js 87.09% <0.00%> (-0.41%) ⬇️
lib/plugins/aws/package/compile/functions.js 94.78% <0.00%> (-0.41%) ⬇️
lib/plugins/package/package.js 95.83% <0.00%> (-0.17%) ⬇️
... and 22 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@overbit great thanks for that! I have just one comment

lib/plugins/aws/package/compile/events/kafka.js Outdated Show resolved Hide resolved
@overbit overbit force-pushed the add-kafka-event-startingPositionTimestamp branch 2 times, most recently from 43e3096 to 315796e Compare October 27, 2022 14:18
@overbit overbit force-pushed the add-kafka-event-startingPositionTimestamp branch from 315ca5c to c6b6b45 Compare October 27, 2022 19:48
@overbit overbit force-pushed the add-kafka-event-startingPositionTimestamp branch from c6b6b45 to cebb796 Compare October 27, 2022 20:23
@overbit overbit force-pushed the add-kafka-event-startingPositionTimestamp branch from 6529aeb to 76dbad8 Compare October 27, 2022 21:09
@overbit overbit changed the title feat: Add support for kafka event startingPositionTimestamp feat: add support for kafka event startingPositionTimestamp Oct 28, 2022
@overbit
Copy link
Contributor Author

overbit commented Nov 1, 2022

@medikoo shall we change the state from Request for changes?

@medikoo
Copy link
Contributor

medikoo commented Nov 2, 2022

@medikoo shall we change the state from

@overbit if you finalized update and PR is ready for re-review, simply mark it on GitHub as such (in reviewers box you can re-reqeuest review)

@overbit overbit requested a review from medikoo November 2, 2022 13:48
@overbit overbit requested a review from medikoo November 2, 2022 14:41
Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @overbit! We're on a right track :)

lib/plugins/aws/package/compile/events/kafka.js Outdated Show resolved Hide resolved
lib/plugins/aws/package/compile/events/kafka.js Outdated Show resolved Hide resolved
@overbit overbit force-pushed the add-kafka-event-startingPositionTimestamp branch from 3b99ca3 to f7ecb9f Compare November 3, 2022 21:31
@overbit overbit requested a review from medikoo November 3, 2022 21:37

if (
startingPosition === 'AT_TIMESTAMP' &&
!(startingPositionTimestamp !== undefined && startingPositionTimestamp !== null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep it simple as startingPosition === 'AT_TIMESTAMP' && startingPositionTimestamp == null

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

@@ -243,6 +257,10 @@ class AwsCompileKafkaEvents {
};
}

if (startingPositionTimestamp !== undefined && startingPositionTimestamp !== null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, let's use simpler condition

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

@overbit overbit force-pushed the add-kafka-event-startingPositionTimestamp branch from f7ecb9f to 3b8b94a Compare November 4, 2022 14:19
@overbit overbit requested a review from medikoo November 4, 2022 14:19
@overbit overbit marked this pull request as draft November 4, 2022 14:35
Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test failed

@overbit overbit force-pushed the add-kafka-event-startingPositionTimestamp branch from 3b8b94a to 47654a3 Compare November 5, 2022 16:29
@overbit overbit marked this pull request as ready for review November 5, 2022 16:37
@overbit overbit requested a review from medikoo November 5, 2022 16:37
Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @overbit !

@medikoo medikoo merged commit 858758e into serverless:main Nov 7, 2022
@overbit overbit deleted the add-kafka-event-startingPositionTimestamp branch November 10, 2022 07:50
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.

Kafka - StartingPosition / StartingPositionTimestamp
2 participants