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

Replace curl with wget (Addresses #2675) #2676

Closed
wants to merge 1 commit into from
Closed

Conversation

vexdev
Copy link

@vexdev vexdev commented Nov 9, 2024

Replace curl with wget as curl is not available from the base image.
Simple proposal to address #2675

Replace curl with wget as curl is not available from the base image.
@@ -28,4 +28,4 @@ CMD ["java", "-cp", "app:app/lib/*", "io.tolgee.Application"]

# Health check to ensure the app is up and running
HEALTHCHECK --interval=10s --timeout=3s --retries=20 \
CMD curl -f http://127.0.0.1:$HEALTHCHECK_PORT/actuator/health || exit 1
CMD wget -q -O - http://127.0.0.1:$HEALTHCHECK_PORT/actuator/health || exit 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for your contribution, it looks working :)

Minor comment: wget can be called with the --spider parameter not to download the content, just check the HTTP code.

Also the || exit 1 is redundant, since it is the default wget functionality.

Suggested change
CMD wget -q -O - http://127.0.0.1:$HEALTHCHECK_PORT/actuator/health || exit 1
CMD wget --spider -q http://127.0.0.1:$HEALTHCHECK_PORT/actuator/health

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is not redundant; wget doesn't always exit with exit code 1 which is the expected code for unhealthy containers, not just non-zero. While it works, the Dockerfile reference explicitly expects an exit code 1, reserves the exit code 2, and makes no mention of other exit codes making them effectively undefined behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

@StaNov Can you please finalize this so we can merge it? 🙏

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have to create a new PR because I cannot work on this branch.

@vexdev FYI, if you were able to finish it in a matter of hours, I'd leave it to you.

@cyyynthia, thanks for the explanation, I didn't know this healthcheck contract.

Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI, I created a new PR #2694. I credited @vexdev in the description.

Copy link
Author

Choose a reason for hiding this comment

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

No worries I'm not after credits only helpful contributions when I can :)

JanCizmar pushed a commit that referenced this pull request Nov 14, 2024
We use `wget` because `curl` is not installed on the image.

Originally developed by @vexdev in #2676, thank you for that!

Closes #2675.


![image](https://github.com/user-attachments/assets/bdd7c689-1747-48fa-a59f-104a3851e97e)

Co-authored-by: StaNov <[email protected]>
@JanCizmar
Copy link
Contributor

It's fixed here: #2694

Thanks to all for contributions!

@JanCizmar JanCizmar closed this Nov 14, 2024
TolgeeMachine pushed a commit that referenced this pull request Nov 15, 2024
## [3.81.4](v3.81.3...v3.81.4) (2024-11-15)

### Bug Fixes

* dashboard translations ([#2697](#2697)) ([88e8465](88e8465))
* Healthcheck of docker image ([#2694](#2694)) ([c21575f](c21575f)), closes [#2676](#2676) [#2675](#2675)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants