Why doesn't url function for wait.ForSQL() have a host parameter?

[frozenbonito]

I think host parameter is required for building a DSN string.

e.g.

	req := testcontainers.ContainerRequest{
		Image: "mysql:8.0",
		// ...
		WaitingFor: wait.ForSQL("3306", "mysql", func(host string, port nat.Port) string {
			cfg := mysql.NewConfig()
			cfg.Net = "tcp"
			cfg.Addr = net.JoinHostPort(host, port.Port())
			// ...
			return cfg.FormatDSN()
		}),
	}

[mdelapenya]

Hi @frozenbonito, thanks for opening this question. Did you check the existing test cases that we have on that? Both https://github.com/testcontainers/testcontainers-go/blob/main/e2e/container_test.go and https://github.com/testcontainers/testcontainers-go/blob/main/wait/sql_test.go could help you, as they demonstrate its usage.

Could you please elaborate a bit more your use case and why you need the host? If it's not supported at this moment, would you be willing to contribute it?

[frozenbonito]

@mdelapenya
Thank you for comment.

I think that the host of the container may not be localhost, for example if TC_HOST env variable is required.
HTTPStragety supports this case because target.Host() is used in WaitUntilReady().

testcontainers-go/wait/http.go

Line 124 in a99f307

ipAddress, err := target.Host(ctx)

https://pkg.go.dev/github.com/testcontainers/testcontainers-go@v0.13.0#DockerContainer.Host

If it's not supported at this moment, would you be willing to contribute it?

It is a breaking change, but if that is okay with you, I can contribute to this issue.

[mdelapenya]

It is a breaking change, but if that is okay with you, I can contribute to this issue.

Thanks for marking it as a breaking change, we do care about them, and we appreciate your effort looking for the usages.

Because the library is not in v1 yet, which is something we plan in the short/medium term, a well-documented breaking change would be something to accept, so feel free to send a contribution, we'll be more than happy for reviewing it!