Skip to content

Conversation

displaylink-kstrzemp
Copy link
Contributor

Motivation

Fix issue: #11259

Changes

Relaxed time constraints in healthcheck

@localstack-bot
Copy link
Contributor

localstack-bot commented Dec 23, 2024

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

Copy link
Contributor

@localstack-bot localstack-bot left a comment

Choose a reason for hiding this comment

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

Welcome to LocalStack! Thanks for raising your first Pull Request and landing in your contributions. Our team will reach out with any reviews or feedbacks that we have shortly. We recommend joining our Slack Community and share your PR on the #community channel to share your contributions with us. Please make sure you are following our contributing guidelines and our Code of Conduct.

@displaylink-kstrzemp
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@alexrashed
Copy link
Member

recheck

@alexrashed alexrashed added the semver: patch Non-breaking changes which can be included in patch releases label Jan 7, 2025
localstack-bot added a commit that referenced this pull request Jan 7, 2025
@alexrashed alexrashed requested a review from bentsku January 7, 2025 12:31
Copy link
Contributor

@bentsku bentsku left a comment

Choose a reason for hiding this comment

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

Thanks a lot for opening a PR and providing a fix for the issue. Looking at the logs of the linked issue, it seems the command was taking around 5.2 seconds to run, leading into the timeout issue.

I think the 10s timeout is a good change, as the interval is 10s anyway. However, diminishing the number of retries might lead to unexpected changes for other users, as before the container had around 15s of start period + 4 interval of 10s + last retry of 5s timeout, and it will now be 15s + 2 interval of 10s + last retry of 10s.

Would you mind reverting the change of retries and then the PR would be good to merge. Thanks again for your contribution 🙏

@displaylink-kstrzemp
Copy link
Contributor Author

displaylink-kstrzemp commented Jan 7, 2025

Would you mind reverting the change of retries and then the PR would be good to merge. Thanks again for your contribution 🙏

done

Copy link
Contributor

@bentsku bentsku left a comment

Choose a reason for hiding this comment

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

LGTM! Awesome, thanks a lot for being so reactive in pushing the changes. Sorry for the delay in reviewing this PR with the holiday season. Thanks again for your PR, and welcome to the contributors of LocalStack!

@bentsku bentsku merged commit edf600e into localstack:master Jan 7, 2025
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants