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

Log retried errors #2613

Merged
merged 2 commits into from
Jul 4, 2024
Merged

Conversation

ash2k
Copy link
Contributor

@ash2k ash2k commented Jun 29, 2024

What does this PR do?

Add logging of errors that are retried.

Why is it important?

I want to see any unexpected errors when they happen. Otherwise nothing is logged and the test appears to be stuck forever.

Related issues

@ash2k ash2k requested a review from a team as a code owner June 29, 2024 08:47
Copy link

netlify bot commented Jun 29, 2024

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit f5adf1d
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/6686b8ff23116800080bca44
😎 Deploy Preview https://deploy-preview-2613--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.

docker.go Show resolved Hide resolved
docker.go Show resolved Hide resolved
@mdelapenya
Copy link
Member

Could you please update this PR with the changes in #2606 🙏

Thanks!

@ash2k
Copy link
Contributor Author

ash2k commented Jul 2, 2024

Rebased

@ash2k
Copy link
Contributor Author

ash2k commented Jul 3, 2024

Tests are failing with:

  2024/07/02 23:11:31 Failed to pull image: Error response from daemon: toomanyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit, will retry

@mdelapenya
Copy link
Member

Rebased

I'm sorry but I do not see the changes for the lifecycle.go function, adding a retry for checking all exposed ports are already available.

@ash2k
Copy link
Contributor Author

ash2k commented Jul 3, 2024

Hm, I've just checked and I think all commits are there:

Am I missing something?

@mdelapenya
Copy link
Member

Oh sorry, I probably did not explain myself very well. I asked you to check (and update if neeeded) the retry added in #2606, to include the RetryNotify pattern this PR introduces 🙏

@ash2k
Copy link
Contributor Author

ash2k commented Jul 3, 2024

Ah, ok. Done!

lifecycle.go Outdated Show resolved Hide resolved
reaper.go Show resolved Hide resolved
reaper.go Show resolved Hide resolved
@mdelapenya mdelapenya self-assigned this Jul 4, 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!

@mdelapenya mdelapenya merged commit 9cd7bcb into testcontainers:main Jul 4, 2024
108 checks passed
@ash2k ash2k deleted the log-retriable-errors branch July 4, 2024 22:20
@mdelapenya mdelapenya added the chore Changes that do not impact the existing functionality label Jul 5, 2024
mdelapenya added a commit that referenced this pull request Jul 15, 2024
* main:
  fix: log output after context timeout (#2643)
  chore(deps): use "github.com/containerd/platforms" instead (#2638)
  chore(deps): bump google.golang.org/grpc to 1.64.1 (#2635)
  chore(deps): bump certifi from 2024.2.2 to 2024.7.4 (#2631)
  chore: prepare for next minor development cycle (0.33.0)
  chore: use new version (v0.32.0) in modules and examples
  feat: honour go toolchain's verbose flag to print out logs (#2624)
  Fix issues in BuildImage() (#2626)
  docs: indicate version for the Run function (#2627)
  chore: remove duplicated tests for config (#2628)
  Log retried errors (#2613)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Changes that do not impact the existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants