-
Notifications
You must be signed in to change notification settings - Fork 102
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
upgrading dockest to 2.1.0 #94
Conversation
@Nevon would you happen to have any bandwidth to take a look at this small-ish PR? |
This is really an @erikengervall thing, as he's the author of Dockest, but I'll try to make time for this once I finish up #93. |
Got some conflicts to resolve, but otherwise it seems fine. As long as the tests are passing, it should be good to go. |
@@ -32,3 +32,5 @@ services: | |||
|
|||
avro_tools: # https://hub.docker.com/r/coderfi/avro-tools | |||
image: coderfi/avro-tools:1.7.7 | |||
ports: | |||
- '9999:9999' |
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.
Why do we need to expose this port for avro-tools?
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.
There is a bug in dockest I believe where the docker-compose must define a port. I'll look into the root cause and see if there is a workaround that doesn't require this hack.
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.
@erikengervall is this something I should address in dockest, or is there a known workaround?
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.
@erikengervall is this something I should address in dockest, or is there a known workaround?
Hey! There's even a version 3.0.0 of Dockest in the making which will include a number of improvements. Can't tell exactly what the ETA is, but there's a few PRs queued to be merged and some beta testing required before we can release.
I'll look into the root cause and see if there is a workaround that doesn't require this hack.
I'd say this port-hack is okay for now, I could circle back to this piece of code once Dockest v3 is out and this has been addressed 🙂
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.
Sounds good, ill take a look as well.
In this PR
Additional Notes
One use case that came to mind as I was testing PR that it might be nice to add other confluent producers to the integration testing suite to test that the deserializers work with other components.