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(influxdb): Respect custom waitStrategy #2845

Merged
merged 11 commits into from
Nov 20, 2024

Conversation

marcinmilewski93
Copy link
Contributor

What does this PR do?

The purpose of this PR is to respect the provided waitStrategy for the InfluxDB container, rather than overriding it by default. This change allows the use of a custom waitStrategy when specified; if none is provided, the default waitStrategy will be applied, maintaining the same configuration as before this modification.

Why is it important?

In the current implementation, it's not possible to use a custom waitStrategy and in my case the default strategy marks the container as ready too early. The container is still not ready to receive writes as you can see below. Customizing the waitStrategy for example to check the /health status solved this issue. Example log output when container is not ready yet:

2024/10/22 10:38:54 ✅ Container started: d5d6612ccd27
2024/10/22 10:38:54 ⏳ Waiting for container id d5d6612ccd27 image: influxdb:2.0.7. Waiting for: &{timeout:<nil> Log:Listening log_id=[0-9a-zA-Z_~]+ service=tcp-listener transport=http IsRegexp:true Occurrence:1 PollInterval:100ms}
2024/10/22 10:39:04 🔔 Container is ready: d5d6612ccd27
2024-10-22T10:39:23.408+07:00|2|ERROR||,,,testing-invalid|9469|goroutine:1783|internal/runner.go:93|Post "http://localhost:49155/api/v2/write?bucket=bucket1&org=influxdata&precision=ns": read tcp [::1]:43726->[::1]:49155: read: connection reset by peer
2024/10/22 10:39:24 [E]! Write error: Post "http://localhost:49155/api/v2/write?bucket=lbucket1&org=influxdata&precision=ns": read tcp [::1]:55852->[::1]:49155: read: connection reset by peer
2024/10/22 10:39:24 unable to send metrics to InfluxDB. err=Post "http://localhost:49155/api/v2/write?bucket=bucket1&org=influxdata&precision=ns": read tcp [::1]:55852->[::1]:49155: read: connection reset by peer

After this modification different strategy could be employed, for example, to verify if InfluxDB is ready for queries and writes as you can see on below health check response:

{
  "name": "influxdb",
  "message": "ready for queries and writes",
  "status": "pass",
  "checks": [],
  "version": "2.0.7",
  "commit": "2a45f0c037"
}

Related issues

  • None

How to test this PR

I have added a test that uses a custom waitStrategy to verify this behavior.

fix(influxdb): Respect passed waitStrategy
@marcinmilewski93 marcinmilewski93 requested a review from a team as a code owner October 22, 2024 08:22
Copy link

netlify bot commented Oct 22, 2024

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit dc16bfe
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/672a6234841d760008d84c0b
😎 Deploy Preview https://deploy-preview-2845--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.

Copy link
Collaborator

@stevenh stevenh left a comment

Choose a reason for hiding this comment

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

Thanks for PR, tried reproducing but must be missing something

modules/influxdb/influxdb_test.go Outdated Show resolved Hide resolved
modules/influxdb/influxdb_test.go Outdated Show resolved Hide resolved
Co-authored-by: Steven Hartland <stevenmhartland@gmail.com>
Copy link
Collaborator

@stevenh stevenh left a comment

Choose a reason for hiding this comment

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

Looking cleaner, did some testing locally which triggered some additional suggestions.

modules/influxdb/influxdb.go Outdated Show resolved Hide resolved
modules/influxdb/influxdb.go Outdated Show resolved Hide resolved
modules/influxdb/influxdb.go Outdated Show resolved Hide resolved
Move Shutdown check to WithInitDb
mdelapenya
mdelapenya previously approved these changes Oct 31, 2024
Copy link
Member

@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!

Copy link
Collaborator

@stevenh stevenh left a comment

Choose a reason for hiding this comment

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

Almost there, just two suggestions

modules/influxdb/influxdb.go Outdated Show resolved Hide resolved
modules/influxdb/influxdb.go Outdated Show resolved Hide resolved
Co-authored-by: Steven Hartland <stevenmhartland@gmail.com>
Co-authored-by: Steven Hartland <stevenmhartland@gmail.com>
mdelapenya
mdelapenya previously approved these changes Oct 31, 2024
Copy link
Member

@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.

Added a final nit about a comment.

Other than that, LGTM, great job @marcinmilewski93 during the review 🙇

modules/influxdb/influxdb.go Show resolved Hide resolved
@mdelapenya mdelapenya self-assigned this Nov 20, 2024
@mdelapenya mdelapenya added the bug An issue with the library label Nov 20, 2024
@mdelapenya mdelapenya merged commit 83b47ca into testcontainers:main Nov 20, 2024
122 checks passed
@mdelapenya
Copy link
Member

Thanks everybody here for the thorough review. Great job!

mdelapenya added a commit to mdelapenya/testcontainers-go that referenced this pull request Nov 21, 2024
* main: (76 commits)
  fix(influxdb): Respect custom waitStrategy (testcontainers#2845)
  fix: only upload to sonar on ubuntu-latest (testcontainers#2891)
  fix: build artifact name properly (testcontainers#2890)
  fix: do not run sonar upload when ryuk is disabled (testcontainers#2889)
  fix: update GH actions for uploading/downloading artifacts (testcontainers#2888)
  feat(ci): Enable master moby with rootless (testcontainers#2880)
  fix(redpanda): temporary file use
  chore(deps): bump actions/download-artifact from 3.0.2 to 4.1.8 (testcontainers#2676)
  chore(deps): bump actions/upload-artifact from 3.1.3 to 4.4.3 (testcontainers#2885)
  fix!: port forwarding clean up and make private (testcontainers#2881)
  chore: resolve AWS deprecations for localstack (testcontainers#2879)
  docs: fix new lifecycle hooks section (testcontainers#2875)
  fix: host access port instability (testcontainers#2867)
  feat: add build to life cycle hooks (testcontainers#2653)
  fix: typo in containerd integration (testcontainers#2873)
  chore(deps): bump mkdocs-include-markdown-plugin from 6.0.4 to 6.2.2 (testcontainers#2806)
  chore: use testify instead of t.Error (testcontainers#2871)
  ci: enable perfsprint linter (testcontainers#2872)
  chore(deps): bump mkdocs-markdownextradata-plugin from 0.2.5 to 0.2.6 (testcontainers#2807)
  chore: use testcontainers.RequireContainerExec (testcontainers#2870)
  ...
mdelapenya added a commit to mdelapenya/testcontainers-go that referenced this pull request Nov 25, 2024
* main:
  docs: better contribution guidelines (testcontainers#2893)
  fix(influxdb): Respect custom waitStrategy (testcontainers#2845)
mdelapenya added a commit to mtellis2/testcontainers-go that referenced this pull request Dec 11, 2024
* main: (234 commits)
  chore(ci): add Github labels based on PR title (testcontainers#2914)
  chore(gha): Use official setup-docker-action (testcontainers#2913)
  chore(ci): enforce conventional commits syntax in PR titles (testcontainers#2911)
  feat(nats): WithConfigFile - pass a configuration file to nats server (testcontainers#2905)
  chore: enable implicit default logger only in testing with -v (testcontainers#2877)
  fix: container binds syntax (testcontainers#2899)
  refactor(cockroachdb): to use request driven options (testcontainers#2883)
  chore(deps): bump actions/setup-go from 5.0.0 to 5.1.0 (testcontainers#2904)
  chore(deps): bump ossf/scorecard-action from 2.3.1 to 2.4.0 (testcontainers#2903)
  chore(deps): bump test-summary/action from 2.3 to 2.4 (testcontainers#2902)
  feat(wait): strategy walk (testcontainers#2895)
  feat(wait): tls strategy (testcontainers#2896)
  docs: better contribution guidelines (testcontainers#2893)
  fix(influxdb): Respect custom waitStrategy (testcontainers#2845)
  fix: only upload to sonar on ubuntu-latest (testcontainers#2891)
  fix: build artifact name properly (testcontainers#2890)
  fix: do not run sonar upload when ryuk is disabled (testcontainers#2889)
  fix: update GH actions for uploading/downloading artifacts (testcontainers#2888)
  feat(ci): Enable master moby with rootless (testcontainers#2880)
  fix(redpanda): temporary file use
  ...
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.

3 participants