restart update_synapse_for_appservice.sh broken for whatsapp#580
Conversation
improve app service error handling for successful run
|
!testme |
Josue-T
left a comment
Josue-T
left a comment
There was a problem hiding this comment.
Thanks, for the review. To it's good to bring some more improvement on this script but I also would like to stay consistent for the systemd helper call with the other scripts.
| chmod 600 "$service_config_file" | ||
|
|
||
| set +e | ||
| ynh_systemctl --service="$app".service --action=restart --wait_until="Synapse now listening on TCP port $port_synapse_tls" --log_path="/var/log/matrix-$app/homeserver.log" --timeout=300 |
There was a problem hiding this comment.
Ideally I would prefer to keep the same parameter than on the install/upgrade script
Line 248 in 5577f5e
There was a problem hiding this comment.
I don't think these checks in a log file are solid, I've had to echo "somestrings" before because an install was failing when the log messages were changed/not appearing, even when the service was started and active.
There was a problem hiding this comment.
Ok but in this case can you please report the issue so we can fix it everywhere and not just in this script.
There was a problem hiding this comment.
It could be I am having a more general logging issue? I haven't had the log message in that log since a week - maybe I'm trying to solve a me-issue only
if any idea, let me know, we can close for now
There was a problem hiding this comment.
Well at last on my side and on the CI we don't seem to have any issue like this. I mean Yunhost correctly detect when the service started and we don't end on the timeout.
| else | ||
| # Wait a moment and verify service is actually running | ||
| sleep 5 | ||
| if ! systemctl is-active --quiet "$app"; then |
There was a problem hiding this comment.
Not sure that it's more strong, because even if it fail as the service will try to restart depending on when you do the check, it might return that it's active even if it crashed.
There was a problem hiding this comment.
maybe need to put is-failed instead of is-active?
is-active PATTERN... Check whether units are active
is-failed PATTERN... Check whether units are failed
There was a problem hiding this comment.
Well yes maybe, to me we need a strong way to check that the service fully started without crashing at some point and it's from my experience what was doing the current code. Now if this not working as expected maybe we just need to understand why and maybe fix the helper itself.
The idea also of this way we check the startup is that we are sure that synapse fully started when we go out of ynh_systemctl and this time could really depends of servers.
2 participants
improve app service error handling for successful run
Problem
old log location matrix-synapse
broke my whatsapp bridge
Solution
remove those wait for and log path line + added if logic for better exit 1 on fail
PR Status
Automatic tests
Automatic tests can be triggered on https://ci-apps-dev.yunohost.org/ after creating the PR, by commenting "!testme", "!gogogadgetoci" or "By the power of systemd, I invoke The Great App CI to test this Pull Request!". (N.B. : for this to work you need to be a member of the Yunohost-Apps organization)