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

Dump ynh log if an app script fails #687

Merged
merged 8 commits into from May 22, 2019

Conversation

Projects
None yet
4 participants
@maniackcrudelis
Copy link
Contributor

commented Mar 17, 2019

The problem

While playing with ynh_debug, I thought it's too bad to have to use that helper to know why an app script has crashed.

Solution

In case of crash of an app script, print the last 30 lines of the log.

PR Status

It works only from the command line, because I use yunohost-cli.log.
I wonder if there's a variable with the log used, or anything that let you know the environment. I'll have a look to it...
If there's a cleaner way to find that info, would be better though.
I tried to add the file and the line, like ynh_debug, but didn't find a way to do it only for a duplicated BASH_XTRACEFD. It either for all logs or for none.

How to test

Add false wherever you want in a script to force it to fail.

Validation

  • Principle agreement 0/2 :
  • Quick review 0/1 :
  • Simple test 0/1 :
  • Deep review 0/1 :

@YunoHost/apps

@zamentur

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2019

The current log system is not enough ?

As a reminder, this one displays the command or link to launch to get the end of the log file.

Many users were lost when the logs were displayed. As a result, we stopped displaying logs to users (now we have to use --debug) to prefer this log management system that invites the user to go and see.

I understand in this PR that the objective is to help the debug, in this case this feature should only be activated in a development context, right?

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

I understand in this PR that the objective is to help the debug, in this case this feature should only be activated in a development context, right?

In development context, you can use --debug because you know the script is probably going to fail, or use ynh_debug because you know where the script will fail but want to know why.

This PR is more for "normal" user when a script fail. There's often simple solution, because a file wasn't removed before or other common issues. But the complete log is hard to read if you don't know what to look for.
The current log system, with the link to see the complete log, is really interesting to see what happened in the script. But it's mostly for us. Here I would like to show a small piece of log, just before the crash, so an admin could just read why its app has crashed and have a look to the forum or ask immediately for the problem he had.
I see often (even if I'm not on the support chat) error logs with simple errors, or already identified ones asked on the forum just because the user didn't read the log.
So my idea is more to show a trace of what happened just before the crash.

@alexAubin alexAubin added this to the 3.6.x milestone Apr 16, 2019

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

What the fuck ! I got utils from stretch-unstable !
Why do I have such a big diff...

maniackcrudelis added some commits May 7, 2019

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

I hate git...
Never mind...

After discussion with ljf, we decided that, since the log is mainly intended to be seen by power users, we could show it only in CLI.
For an execution from the web admin, a failure would anyway show a link to the log with the last lines of it.
A made also some aesthetic adjustments.
The final result is that:

@Psycojoker
Copy link
Member

left a comment

Principle approval, this is something I already wanted to do in the past but never did.

Show resolved Hide resolved data/helpers.d/utils Outdated

alexAubin added some commits May 18, 2019

Inject a new environment variable YNH_INTERFACE to test inside helper…
…s if we're using Yunohost from the CLI or API
@alexAubin

This comment has been minimized.

Copy link
Member

commented May 18, 2019

During my tests, I added an exit -1 in my install script basically near the very beginning of the script... But then the snippet of code detected the initial setup of ynh_exit_properly :

3798 DEBUG + set -eu
3798 DEBUG + trap ynh_exit_properly EXIT

So the actual error was not shown (Instead, I got lines about helpers being loaded etc).

So to improve the accuracy, in 899d3d4 I propose to add a + such that this line gets caught instead :

3802 DEBUG + ynh_exit_properly

(Maybe there's a better trick like using a $ idk)

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2019

Did you try it later in the script, with an exit at the end for example ?

@alexAubin

This comment has been minimized.

Copy link
Member

commented May 18, 2019

Well then it works yes ;)

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2019

Ok then

@alexAubin
Copy link
Member

left a comment

LGTM 👍

@alexAubin

This comment has been minimized.

Copy link
Member

commented May 21, 2019

Will merge in a few days

@alexAubin

This comment has been minimized.

Copy link
Member

commented May 22, 2019

Actually yolomergin' for 3.6 testing

@alexAubin alexAubin merged commit 6cdffe7 into stretch-unstable May 22, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@alexAubin alexAubin deleted the crash_debugger branch May 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.