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 Dockerfile.s3 healthcheck #10969

Closed
wants to merge 1 commit into from

Conversation

krishanbhasin-px
Copy link
Contributor

@krishanbhasin-px krishanbhasin-px commented Jun 6, 2024

Motivation

The latest-pushed latest-s3 image fails to boot properly for me:

 localstack-s3 Pulled 
 Container localstack-s3-1  Creating
 Container localstack-s3-1  Created
 Container localstack-s3-1  Starting
 Container localstack-s3-1  Started
 Container localstack-s3-1  Waiting
<after 60s timeout passes>
container localstack-s3-1 is unhealthy

Comparing the logs between the old image that worked for us and the new one that doesn't, I can't see any healthcheck calls.

Shelling into the image, manually trying to run the heathcheck command ./bin/localstack status services --format=json fails with ModuleNotFoundError: No module named 'localstack'

Changes

Copied over the healthcheck changes in #10894 to the second dockerfile

I manually tested this by running the latest image and running .venv/bin/localstack status services --format=json, and got a successful response back

Request:

It looks like only latest-* images are being published to dockerhub; would it be possible to return to publishing tagged/pinned versions?

@localstack-bot
Copy link
Collaborator


Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.


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


You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.

Copy link
Collaborator

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

@krishanbhasin-px krishanbhasin-px marked this pull request as ready for review June 6, 2024 09:42
@bentsku bentsku requested a review from alexrashed June 6, 2024 11:03
@bentsku bentsku added the semver: patch Non-breaking changes which can be included in patch releases label Jun 6, 2024
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.

The change looks good to me! Sorry for the inconvenience on this, as you found out, the issue stems from the same issue as #10894 and is fixed the same way.

Regarding the latest-s3 tag being published, for now the S3-only image is not aligned to the regular LocalStack tagged version. However, we can think about it and maybe start releasing tagged version for this kind of image, as it starts to get more traction, in order to give it more stability.

@bentsku
Copy link
Contributor

bentsku commented Jun 6, 2024

@krishanbhasin-px, additionally, could you please accept the CLA by following the instructions in this comment in order for us to be able to merge this PR? #10969 (comment)

Thank you!

@krishanbhasin-px
Copy link
Contributor Author

@bentsku aaah, I need to get signoff from my employer; I expect it should be straightforward but the relevant people aren't available until next week.

@bentsku
Copy link
Contributor

bentsku commented Jun 6, 2024

@krishanbhasin-px ah, I see 😬 Would you mind if I quickly open a PR myself to do the fix in order to unblock the S3 image? We can always merge this PR anyway next week in order for your contribution to get in. Would that be alright?

@krishanbhasin-px
Copy link
Contributor Author

@bentsku yes absolutely!

@bentsku
Copy link
Contributor

bentsku commented Jun 6, 2024

Closing in favor of #10970, thank you again!

@bentsku bentsku closed this Jun 6, 2024
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.

None yet

3 participants