Update Pulsar Container #858

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.

Sign up for GitHub

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

Jump to bottom

Merged

bsideup merged 1 commit into testcontainers:master from aahmed-se:pulsar_update1

Sep 13, 2018

Contributor

aahmed-se commented Sep 5, 2018 •

edited

Loading

Simply startup script using defaults.

aahmed-se requested review from bsideup, kiview and rnorth as code owners

September 5, 2018 21:38

Member

bsideup commented Sep 6, 2018

Hi @aahmed-se! It seems that 2.2.0 is not released yet :D

aahmed-se changed the title ~~Update Pulsar Container~~ [WIPUpdate Pulsar Container

Contributor Author

aahmed-se commented Sep 6, 2018

my mistake it's actually another change for something else, will mark it wip for now

aahmed-se changed the title ~~[WIPUpdate Pulsar Container~~ [WIP] Update Pulsar Container

aahmed-se force-pushed the pulsar_update1 branch from b714d28 to 4b1dc1c Compare

September 9, 2018 01:30

Contributor Author

aahmed-se commented Sep 9, 2018

This is ready for review now

aahmed-se changed the title ~~[WIP] Update Pulsar Container~~ Update Pulsar Container

aahmed-se force-pushed the pulsar_update1 branch from 4b1dc1c to c97fa9f Compare

September 9, 2018 05:38

bsideup requested changes

View reviewed changes

modules/pulsar/src/main/java/org/testcontainers/containers/PulsarContainer.java Outdated

                   public PulsarContainer() {
-                      this("2.1.0-incubating");
+                      super(IMAGE_NAME);

Member

bsideup Sep 9, 2018

could you please revert constructor removal? That's a breaking change

modules/pulsar/src/main/java/org/testcontainers/containers/PulsarContainer.java Outdated

-                      waitingFor(Wait.forLogMessage(".*messaging service is ready.*\\s", 1));
+                  @Override
+                  public void start() {
+                      this.waitStrategy = new HttpWaitStrategy()

Member

bsideup Sep 9, 2018

please use waitingFor during the configuration

modules/pulsar/src/main/java/org/testcontainers/containers/PulsarContainer.java Outdated

+               * This container wraps Apache Pulsar running in standalone mode
                */
-              public class PulsarContainer extends GenericContainer<PulsarContainer> {
+              public class PulsarContainer<T extends PulsarContainer<T>> extends GenericContainer<T> {

Member

bsideup Sep 9, 2018

this container is a final container, there is no need for the self-typing

bsideup added this to the next milestone

bsideup added type/feature type/dependency update labels

aahmed-se force-pushed the pulsar_update1 branch from c97fa9f to 45e6860 Compare

September 10, 2018 01:28

Contributor Author

aahmed-se commented Sep 10, 2018

Comments addressed

bsideup reviewed

View reviewed changes

modules/pulsar/src/main/java/org/testcontainers/containers/PulsarContainer.java Outdated

-                      waitingFor(Wait.forLogMessage(".*messaging service is ready.*\\s", 1));
+                      withExposedPorts(BROKER_HTTP_PORT, BROKER_PORT);
+                      withCommand("/bin/bash", "-c", "/pulsar/bin/pulsar standalone --no-functions-worker -nss");
+                      waitingFor(Wait.forHttp(METRICS_ENDPOINT).forStatusCode(200));

Member

bsideup Sep 10, 2018

would be better to specify port here explicitly, otherwise it uses "first exposed port" and users might change exposed ports according to their needs

bsideup reviewed

View reviewed changes

modules/pulsar/src/main/java/org/testcontainers/containers/PulsarContainer.java Outdated

-                      waitingFor(Wait.forLogMessage(".*messaging service is ready.*\\s", 1));
+                      withExposedPorts(BROKER_HTTP_PORT, BROKER_PORT);
+                      withCommand("/bin/bash", "-c", "/pulsar/bin/pulsar standalone --no-functions-worker -nss");

Member

bsideup Sep 10, 2018

fyi should also work with withCommand("/pulsar/bin/pulsar standalone", "--no-functions-worker", "-nss");


          Update Pulsar Container

47243e1

aahmed-se force-pushed the pulsar_update1 branch from 45e6860 to 47243e1 Compare

September 10, 2018 15:51

Contributor Author

aahmed-se commented Sep 10, 2018

Made the changes

Contributor Author

aahmed-se commented Sep 11, 2018

@bsideup let me know if there any more comments.

bsideup modified the milestones: 1.9.0-rc1, next

Contributor Author

aahmed-se commented Sep 13, 2018

@bsideup is there anything else to change ?

bsideup approved these changes

View reviewed changes

bsideup merged commit 66e89ec into testcontainers:master

Member

bsideup commented Sep 13, 2018

@aahmed-se it looks great, thank you 👍

aahmed-se deleted the pulsar_update1 branch

September 13, 2018 08:12

Member

rnorth commented Sep 21, 2018

Released for preview in 1.9.0-rc2, to be published on Bintray.

Thanks @aahmed-se!

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

Labels