Skip to content

Conversation

@joebowbeer
Copy link
Contributor

@joebowbeer joebowbeer commented Sep 10, 2024

Fixes #833

Moves the resolveHostname invocation back to the beforeContainerCreated method.

Fixes the networkAliases override in resolveHostname - to now call withEnvironment.

Also removes trailing " from the resolveHostname log message and encloses the value assigned to LOCALSTACK_HOST in quotes. Sample messages:

testcontainers [INFO] LOCALSTACK_HOST environment variable set to "myhost" (explicitly as environment variable)

testcontainers [INFO] LOCALSTACK_HOST environment variable set to "myalias" (to match last network alias on container with non-default network)

testcontainers [INFO] LOCALSTACK_HOST environment variable set to "localhost" (to match host-routable address for container)

@netlify
Copy link

netlify bot commented Sep 10, 2024

Deploy Preview for testcontainers-node ready!

Name Link
🔨 Latest commit 9516955
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-node/deploys/66e04463bdac790008a2eb64
😎 Deploy Preview https://deploy-preview-834--testcontainers-node.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@cristianrgreco cristianrgreco added bug Something isn't working patch Backward compatible bug fix labels Sep 10, 2024
@cristianrgreco cristianrgreco changed the title fix resolve hostname Fix LocalStack container hostname resolution Sep 10, 2024
@cristianrgreco
Copy link
Collaborator

cristianrgreco commented Sep 10, 2024

@joebowbeer Could you have a look at this failing test?

FAIL packages/modules/localstack/src/localstack-container.test.ts (47.116 s)
  LocalStackContainer
    ✓ should create a S3 bucket (26507 ms)
    ✓ should use custom network (14459 ms)
    ✕ should assign last network alias to LOCALSTACK_HOST by default (1365 ms)

  ● LocalStackContainer › should assign last network alias to LOCALSTACK_HOST by default

    expect(received).toContain(expected) // indexOf

    Expected substring: "myhost"
    Received string:    ""

      77 |
      78 |     const { output } = await container.exec(["printenv", "LOCALSTACK_HOST"]);
    > 79 |     expect(output).toContain("myhost");
         |                    ^
      80 |
      81 |     await container.stop();
      82 |   });

      at Object.<anonymous> (src/localstack-container.test.ts:79:20)

@joebowbeer
Copy link
Contributor Author

joebowbeer commented Sep 10, 2024

@cristianrgreco Ideas?

As expected, I can see resolveHostname being called in the new (old) location, the variable assignment being added to the environment, and the log message (NOTE correction) being emitted:

testcontainers [INFO] LOCALSTACK_HOST environment variable set to "myhost" (to match last network alias on container with non-default network)

But the exec is returning exitCode 1 😞 which indicates that the overridden assignment is not being passed through. (The printenv exitCode is 1 if the variable is undefined.)

@joebowbeer joebowbeer force-pushed the joebowbeer-patch1 branch 2 times, most recently from a96f671 to f64563a Compare September 10, 2024 11:14
@joebowbeer
Copy link
Contributor Author

@cristianrgreco PTAL

@joebowbeer joebowbeer mentioned this pull request Sep 10, 2024
@cristianrgreco cristianrgreco merged commit 0e4a798 into testcontainers:main Sep 11, 2024
@joebowbeer joebowbeer deleted the joebowbeer-patch1 branch September 11, 2024 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working patch Backward compatible bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

localstack: constructor ignores LOCALSTACK_HOST

2 participants