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

Fails if TELEGRAM_STAT is not set #30

Closed
noplanman opened this issue Dec 20, 2020 · 14 comments · Fixed by #50
Closed

Fails if TELEGRAM_STAT is not set #30

noplanman opened this issue Dec 20, 2020 · 14 comments · Fixed by #50
Labels
bug Something isn't working

Comments

@noplanman
Copy link

For me, using docker, the container fails if the TELEGRAM_STAT variable isn't set.

Maybe because the healthcheck uses it but it's not available?

@MarcoBuster MarcoBuster added this to To do in TDLight bot api via automation Dec 21, 2020
@MarcoBuster MarcoBuster removed this from To do in TDLight bot api Dec 21, 2020
@MarcoBuster MarcoBuster added the bug Something isn't working label Dec 21, 2020
@MarcoBuster
Copy link
Collaborator

Can you send us the logs please?

@noplanman
Copy link
Author

Hi @MarcoBuster, which logs?

When the container starts, the status is starting for a long time, then it says unhealthy. I can't connect to the API and simply get a "404 page not found".

The container logs only have a single line with the command for starting the API server, nothing more.

@MarcoBuster
Copy link
Collaborator

@noplanman set TELEGRAM_VERBOSITY=4 then the logs will appear. Without them we can't understand the problem.

@luckydonald
Copy link
Collaborator

@MarcoBuster Isn't the health check like a curl request to the stats port?

It simply says "curl -f http://localhost:8082/ || exit 1"
In the Dockerfile

Of cause that'll fail if there's no :8082 port serving the stats.
The locks would be the wrong place to look here anyway, and I think it's not needed; that's some simple logic error.
As we now have the ping command, we should use that one instead of the stats endpoint.

@luckydonald
Copy link
Collaborator

@luckydonald *logs
(Sorry am on mobile)

@giuseppeM99
Copy link
Collaborator

@luckydonald the ping command requires a valid token

@luckydonald
Copy link
Collaborator

@luckydonald Wait, we can't use a ping command as that is bound to a bot so we would need to add a bot token?

@luckydonald
Copy link
Collaborator

@luckydonald So to fix it properly, we should either so if the ping port without a token (unauthorized) as well.
Maybe for now, if the stats are not explicitly enabled they could be instead be enabled but bound to just the local network interface needed to get the curl request done?

@spontanurlaub
Copy link
Collaborator

@luckydonald Theoretically ping doesn't require a token, we could move the ping command out of there, but it is more confusing with the URL layout then.

@MarcoBuster
Copy link
Collaborator

...or we could just disable the health check if you don't have TELEGRAM_STAT set

@giuseppeM99
Copy link
Collaborator

@MarcoBuster is that even possible?

@MarcoBuster
Copy link
Collaborator

@giuseppeM99 I don't know, we should investigate

@giuseppeM99
Copy link
Collaborator

@MarcoBuster i'll fix it asap

@luckydonald
Copy link
Collaborator

luckydonald commented Jan 4, 2021

Okey, just to give my two cents, maybe we should actually look into creating a real /healtcheck endpoint.
So if it does a ping or not, would be a implementation detail.
Maybe that could even be served at the root directly, doing a few non-critical stats there (See #37?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants