Skip to content

Conversation

@george-of-croton
Copy link

@george-of-croton george-of-croton commented Jul 16, 2025

it's valid to connect to nats with no username or password. This PR adds a method to remove credential flags on startup.

@netlify
Copy link

netlify bot commented Jul 16, 2025

Deploy Preview for testcontainers-node ready!

Name Link
🔨 Latest commit 8570b79
🔍 Latest deploy log https://app.netlify.com/projects/testcontainers-node/deploys/687852770d32bf00082001c6
😎 Deploy Preview https://deploy-preview-1077--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 project configuration.

@george-of-croton george-of-croton changed the title Allow no credentials nats Allow initialising nats server with no credentials set Jul 16, 2025
@cristianrgreco
Copy link
Collaborator

cristianrgreco commented Jul 16, 2025

The credentials are already handled transparently - what's the use case for explicitly not setting them?

@george-of-croton
Copy link
Author

The credentials are already handled transparently - what's the use case for explicitly not setting them?

I'm working on a project that runs nats as a message bus in k8s cluster that doesn't use username / password auth. I can't use this test container because projects library doesn't expect username and password to be provided.

@daniel-zhang-easygo
Copy link

+1, we use nats with TLS. In unit tests we'd like to disable auth entirely. If we use username+password we'd have to introduce a variant into the NATS client code solely to use the test container which is not ideal.

@cristianrgreco cristianrgreco requested a review from Copilot July 18, 2025 10:56
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enables starting a NATS server container without credentials, which is a valid NATS configuration. It adds a method to control whether credentials are used during container startup.

  • Added withCredentials(boolean) method to enable/disable credential usage
  • Modified container startup to conditionally pass credentials to the started container
  • Added test coverage for the no-credentials scenario

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
packages/modules/nats/src/nats-container.ts Implements credential control logic and conditional credential passing
packages/modules/nats/src/nats-container.test.ts Adds test for no-credentials functionality
docs/modules/nats.md Documents the new no-credentials feature

Comment on lines +87 to +88
(this.useCredentials && this.getUser()) || undefined,
(this.useCredentials && this.getPass()) || undefined
Copy link

Copilot AI Jul 18, 2025

Choose a reason for hiding this comment

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

[nitpick] The conditional logic could be simplified by using a ternary operator for better readability: this.useCredentials ? this.getPass() : undefined

Suggested change
(this.useCredentials && this.getUser()) || undefined,
(this.useCredentials && this.getPass()) || undefined
this.useCredentials ? this.getUser() : undefined,
this.useCredentials ? this.getPass() : undefined

Copilot uses AI. Check for mistakes.
return this;
}

public withCredentials(useCredentials: boolean): this {
Copy link
Collaborator

@cristianrgreco cristianrgreco Jul 18, 2025

Choose a reason for hiding this comment

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

@george-of-croton Need to handle this scenario:

const container = await
  .withUsername("x")
  .withUsername("y")
  .withCredentialsEnabled(true)
  .start()

container.getUser(); // test
container.getPass(); // test

@cristianrgreco cristianrgreco added enhancement New feature or request minor Backward compatible functionality labels Jul 22, 2025
@cristianrgreco
Copy link
Collaborator

@george-of-croton this PR is getting stale, are you planning on addressing the review comments?

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.

3 participants