Skip to content
This repository was archived by the owner on Mar 23, 2026. It is now read-only.

disable health check for default service providers#9202

Merged
thrau merged 1 commit into
masterfrom
disable-health-check
Sep 22, 2023
Merged

disable health check for default service providers#9202
thrau merged 1 commit into
masterfrom
disable-health-check

Conversation

@thrau
Copy link
Copy Markdown
Member

@thrau thrau commented Sep 21, 2023

Motivation

For legacy reasons, we used the local_api_checker as default health check. This made sense when we had backends running on different ports, but now almost all providers run in memory and not as backends, and those that require backends (like dynamodb), aren't considered anymore through the default local_api_checker health check (since we basically just call :4566).
Basically, doing health checks in this way makes no sense any more, and we can evaluate whether the concept as such makes sense and if so what we need to do to perform meaningful health checks.

I've seen errors related to these health checks in logs recently, and I think this is a good opportunity to get rid of them.

Changes

  • When using Service.for_provider (which is done in most cases), disable the health check

@thrau thrau requested a review from alexrashed September 21, 2023 17:44
@thrau thrau added the semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases label Sep 21, 2023
@simonrw
Copy link
Copy Markdown
Contributor

simonrw commented Sep 21, 2023

Does this PR invalidate #9200?

@thrau
Copy link
Copy Markdown
Member Author

thrau commented Sep 21, 2023

Does this PR invalidate #9200?

thanks for making me aware of that one. actually i suppose it would. your effort to fix this is much more admirable than what i'm doing 😄 but i think we should just get rid of them.

@coveralls
Copy link
Copy Markdown

Coverage Status

coverage: 82.967% (+0.03%) from 82.94% when pulling 5e9bf91 on disable-health-check into 623e383 on master.

@github-actions
Copy link
Copy Markdown

LocalStack Community integration with Pro

       2 files  ±0         2 suites  ±0   1h 15m 8s ⏱️ - 1m 37s
2 217 tests ±0  1 721 ✔️ ±0  496 💤 ±0  0 ±0 
2 218 runs  ±0  1 721 ✔️ ±0  497 💤 ±0  0 ±0 

Results for commit 5e9bf91. ± Comparison against base commit 623e383.

Copy link
Copy Markdown
Member

@alexrashed alexrashed left a comment

Choose a reason for hiding this comment

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

Awesome! This will help decreasing the coupling with localstack-python even further. I think we can remove this legacy code with the next major release. 🚀

@thrau thrau added this to the 2.3 milestone Sep 22, 2023
@thrau thrau merged commit f77d0b3 into master Sep 22, 2023
@thrau thrau deleted the disable-health-check branch September 22, 2023 09:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants