-
Notifications
You must be signed in to change notification settings - Fork 5
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 the ability to set specific images via ZeebeClusterBuilder #143
feat: add the ability to set specific images via ZeebeClusterBuilder #143
Conversation
Hey @npepinpe! I see that you are the main contributor to this project, can you start workflows? :) |
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.
Hi Alexey, thanks for the PR! This seems like a reasonable feature - it's a little cumbersome having to iterate over brokers/gateways after building the cluster to set these properties. I think we may want to take a different approach to customizing them eventually - but as this is an experimental API, there's no reason for us not to play around with different approaches.
One small comment on the tests: I found that describing the assertions help a lot with readability, especially for others. You'll notice almost all assertions have an as
method which gives more context. It's important when writing tests not only to ensure they're correct, but also that when they fail, we provide as much context as possible so the person looking at it (who may not have written the test initially) knows what needs to be fixed.
src/main/java/io/zeebe/containers/cluster/ZeebeClusterBuilder.java
Outdated
Show resolved
Hide resolved
src/main/java/io/zeebe/containers/cluster/ZeebeClusterBuilder.java
Outdated
Show resolved
Hide resolved
src/main/java/io/zeebe/containers/cluster/ZeebeClusterBuilder.java
Outdated
Show resolved
Hide resolved
src/test/java/io/zeebe/containers/cluster/ZeebeClusterBuilderTest.java
Outdated
Show resolved
Hide resolved
Expand assertions to the `singleElement` and `satisfies`
Hey @npepinpe. Can you take another look? |
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.
Looks good!
Description
I've added the convenience methods to set a docker image for the Zeebe container in the cluster.
Related issues
closes #139
Pull Request Checklist
mvn clean install -DskipTests
locally before committing