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

Remove explicit port binding in Azurite container #876

Merged

Conversation

ChlorUpload
Copy link
Contributor

@ChlorUpload ChlorUpload commented Nov 28, 2024

Fixes #875.

Switched from explicit port binding to automatic assignment to avoid port conflicts.

Switched from explicitly binding ports to relying on automatic assignment to avoid manual management of ports. Simplifies configuration and reduces potential conflicts.
Copy link

netlify bot commented Nov 28, 2024

Deploy Preview for testcontainers-node ready!

Name Link
🔨 Latest commit 5ba0d8e
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-node/deploys/674bfd92a880be00080e371a
😎 Deploy Preview https://deploy-preview-876--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 enhancement New feature or request minor Backward compatible functionality labels Nov 28, 2024
{ container: QUEUE_PORT, host: this.queuePort },
{ container: TABLE_PORT, host: this.tablePort }
);
this.withCommand(command).withExposedPorts(BLOB_PORT, QUEUE_PORT, TABLE_PORT);
Copy link
Collaborator

@cristianrgreco cristianrgreco Nov 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The withExposedPorts should move to the constructor. If we keep it in the start method, then it cannot be overridden by users.

You should be able to verify this by re-enabling the should be able to specify custom ports test and providing

.withExposedPorts(
      { container: BLOB_PORT, host: this.blobPort },
      { container: QUEUE_PORT, host: this.queuePort },
      { container: TABLE_PORT, host: this.tablePort }
    );

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If developers are allowed to use withExposedPort(), it becomes difficult to create the necessary endpoints as the port order for blob, queue, and table can be set arbitrarily.

Appreciating the existing implementation, the withBlobPort, withQueuePort, and withTablePort functions were modified to accept PortWithOptionalBinding. By using hasHostBinding, it distinguishes whether the port is custom or automatically mapped. Could you review this again? 🙏

@cristianrgreco
Copy link
Collaborator

cristianrgreco commented Nov 30, 2024

Tests failing:

FAIL packages/modules/azurite/src/azurite-container.test.ts (38.5 s)
  Azurite
    ✓ should upload and download blob with default credentials (10494 ms)
    ✓ should add to queue with default credentials (1322 ms)
    ✓ should add to table with default credentials (1306 ms)
    ✓ should be able to specify accountName and accountKey (1356 ms)
    ✕ should be able to specify custom ports (944 ms)
    ✕ should be able to use in-memory persistence (18332 ms)

  ● Azurite › should be able to specify custom ports

    expect(received).toBe(expected) // Object.is equality

    Expected: 13000
    Received: 32783

      125 |       .start();
      126 |
    > 127 |     expect(container.getBlobPort()).toBe(blobPort);
          |                                     ^
      128 |     expect(container.getQueuePort()).toBe(queuePort);
      129 |     expect(container.getTablePort()).toBe(tablePort);
      130 |

      at Object.<anonymous> (src/azurite-container.test.ts:127:37)

  ● Azurite › should be able to use in-memory persistence

    RestError: connect ECONNREFUSED ::1:32786

      at ClientRequest.<anonymous> (../../../node_modules/@azure/core-rest-pipeline/src/nodeHttpClient.ts:223:11)

@ChlorUpload
Copy link
Contributor Author

ChlorUpload commented Dec 1, 2024

  • Fixed the 'Azurite › should be able to specify custom ports' test.
  • In the 'Azurite › should be able to use in-memory persistence' test, container.refresh() now binds a new ports. So, I modified the test to create an azurite client from a new connection string including the new ports.

@cristianrgreco cristianrgreco merged commit 1be6052 into testcontainers:main Dec 2, 2024
167 checks passed
@ChlorUpload ChlorUpload deleted the azurite-no-explicit-port branch December 5, 2024 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request minor Backward compatible functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AzuriteContainer does not map ports. Is it intentional?
2 participants