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

Fix: HTTP wait strategy does not take query params into account #2466

Merged

Conversation

benja-M-1
Copy link
Contributor

@benja-M-1 benja-M-1 commented Apr 4, 2024

What does this PR do?

When specifying a wait strategy using HTTP with query params, they are not sent to the container.
For example, to ensure the elasticmq container is ready, as advised in this issue, one must
poll the /?Action=ListQueues endpoint.
Currently this returns a 400 Bad request because the query parameters are not sent correctly to the endpoint.

The way the fix work is to parse the given path to create a url.URL instance and update the fields with the other values. This way the query parameters and the path are correclty handled.

Why is it important?

Related issues

Here is a reproduction with the elasticmq container:

req := testcontainers.ContainerRequest{
	Image:        "softwaremill/elasticmq-native:1.5.7",
	ExposedPorts: []string{"9324/tcp"},
	WaitingFor:   wait.ForHTTP("/?Action=ListQueues").WithPort("9324").WithStartupTimeout(3 * time.Second),
}

genericContainerReq := testcontainers.GenericContainerRequest{
	ContainerRequest: req,
	Started:          true,
}

c, err := testcontainers.GenericContainer(ctx, genericContainerReq)
if err != nil {
	return fmt.Errorf("failed to start repos-new elasticmq container: %w", err)
}

@benja-M-1 benja-M-1 requested a review from a team as a code owner April 4, 2024 11:04
Copy link

netlify bot commented Apr 4, 2024

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit d24da57
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/660e894e37784000080b8855
😎 Deploy Preview https://deploy-preview-2466--testcontainers-go.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.

@mdelapenya mdelapenya self-assigned this Apr 4, 2024
@mdelapenya mdelapenya added the bug An issue with the library label Apr 4, 2024
Copy link
Collaborator

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@mdelapenya mdelapenya merged commit 4a079a0 into testcontainers:main Apr 4, 2024
102 checks passed
@benja-M-1 benja-M-1 deleted the fix/wait-http-query-params branch April 4, 2024 13:00
mdelapenya added a commit to coffeegoddd/testcontainers-go that referenced this pull request Apr 12, 2024
* main: (115 commits)
  chore: create TLS certs in a consistent manner (testcontainers#2478)
  chore(deps): bump idna from 3.6 to 3.7 (testcontainers#2480)
  Elasticsearch disable CA retrieval when ssl is disabled (testcontainers#2475)
  fix: handle dockerignore exclusions properly (testcontainers#2476)
  chore: prepare for next minor development cycle (0.31.0)
  chore: use new version (v0.30.0) in modules and examples
  Fix url creation to handle query params when using HTTP wait strategy (testcontainers#2466)
  fix: data race on container run (testcontainers#2345)
  fix: logging deadlock (testcontainers#2346)
  feat(k6):Add remote test scripts (testcontainers#2350)
  feat: optimizes file copies to and from containers (testcontainers#2450)
  fix(exec): updates the `Multiplexed` opt to combine stdout and stderr (testcontainers#2452)
  Upgrade neo4j module to use features from v0.29.1 of testcontainers-go (testcontainers#2463)
  bug:Fix AMQPS url (testcontainers#2462)
  chore: more compose updates in comments
  chore: use "docker compose" (v2) instead of "docker-compose" (v1) (testcontainers#2464)
  chore(deps): bump github/codeql-action from 2.22.12 to 3.24.9 (testcontainers#2459)
  refactor: Add Weaviate modules tests (testcontainers#2447)
  feat(exitcode): Add exit code sugar method (testcontainers#2342)
  feat: add module to support InfluxDB v1.x (testcontainers#1703)
  ...
mdelapenya added a commit to stevenh/testcontainers-go that referenced this pull request Apr 24, 2024
* main: (22 commits)
  feat: forward host ports to a container using an SSH tunnel (testcontainers#2471)
  Update follow_logs.md with adding missing package (testcontainers#2513)
  fix: don't retry on permanent APIClient errors (testcontainers#2506)
  feat: support overriding the default recreate options for compose (testcontainers#2511)
  feat: support passing io.Reader for compose files when creating a compose instance (testcontainers#2509)
  chore: add funding button for testcontainers (testcontainers#2510)
  feat: support Ryuk for the compose module (testcontainers#2485)
  chore(deps): bump golang.org/x/net in modules (minio, gcloud, weaviate, compose, qdrant, couchbase, k3s, milvus, mockserver, pulsar, kafka) (testcontainers#2505)
  fix: fallback to URL-path when parsing auth config URL without scheme (testcontainers#2488)
  fix(postgres): Fix the non-default dbname error (testcontainers#2489)
  feat: Bump default postgres version (testcontainers#2481)
  support Dolt (testcontainers#2177)
  chore: create TLS certs in a consistent manner (testcontainers#2478)
  chore(deps): bump idna from 3.6 to 3.7 (testcontainers#2480)
  Elasticsearch disable CA retrieval when ssl is disabled (testcontainers#2475)
  fix: handle dockerignore exclusions properly (testcontainers#2476)
  chore: prepare for next minor development cycle (0.31.0)
  chore: use new version (v0.30.0) in modules and examples
  Fix url creation to handle query params when using HTTP wait strategy (testcontainers#2466)
  fix: data race on container run (testcontainers#2345)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue with the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants