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

add option to set kafka's StartOffset on the consumer #379

Merged
merged 2 commits into from
Feb 11, 2022

Conversation

JohnRoesler
Copy link
Contributor

Description

Exposes a new method WithStartOffset using the Option on the EventBus allowing users to configure where to start the consumer

Affected Components

  • Event Bus, Event Store etc

Related Issues

#339

Solution and Design

I followed the pattern using the Option type similar to WithCodec

Steps to test and verify

  1. Added a unit test to verify the option is set properly

How do you feel about the stretchr testify package? I can remove that for the test assertions if you'd prefer.

@maxekman
Copy link
Member

maxekman commented Feb 9, 2022

Nice PR! I approved running on CI. Please rebase and take a look at the tests and we can merge it.

@JohnRoesler
Copy link
Contributor Author

the test is failing to connect to kafka, which of course I had kafka running locally when i tested so it worked. what's the best strategy here? remove the test? flag it as integration requiring kafka? other?

@maxekman
Copy link
Member

Yeah, flag as integration. Look at the other integration tests for how. 👍

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 68.831% when pulling 30ded5e on JohnRoesler:kafkaStartOffset into 3ed3184 on looplab:main.

@maxekman maxekman merged commit 9c37770 into looplab:main Feb 11, 2022
@maxekman
Copy link
Member

Thanks for the contribution!

@JohnRoesler JohnRoesler deleted the kafkaStartOffset branch February 11, 2022 16:04
@JohnRoesler
Copy link
Contributor Author

You're welcome. I appreciate the library 👍

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

Successfully merging this pull request may close these issues.

None yet

3 participants