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 ynh_check_app_version_changed behavior #756

Open
wants to merge 1 commit into
base: stretch-unstable
from

Conversation

@maniackcrudelis
Copy link
Contributor

commented Jul 14, 2019

The problem

ynh_check_app_version_changed does not kill the upgrade when the app is up to date.
The script says "Up-to-date, nothing to do" and continue with the upgrade nonetheless.

Solution

A subshell can't exit its parent shell, unless with a kill of the parent. Which would be a parricide, not cool...
As well, trap isn't trigger by a subshell. To fix that, I added set -o errtrace

About the main issue, there's no proper way to exit the parent.
Either we could use a variable set into the parent script and run the helper directly without a subshell. In that case exit would work. But we would have a variable to set before, which could be done just before the helper is loaded.

Which means we wouldn't use upgrade_type=$(ynh_check_app_version_changed) but simply ynh_check_app_version_changed into the upgrade script and to have upgrade_type declare before the helper to be a variable set into the main shell.

Or we could use the way I used here, return a value that says that the app is up to date.
That other way implies the upgrade script use that value to not upgrade the app.

Both solutions work, the first one is lighter for the upgrade script, as you can just change the line, but will exit immediately the script, as it was supposed to be.
The second is heavier but allow the upgrade script to deal with the variable, especially to that the upgrade is completed before finishing.

Which solution would you prefer ?

PR Status

Tested with both solutions, only the second one is implement here.

How to test

...

Validation

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

@YunoHost/apps

@kay0u

kay0u approved these changes Jul 18, 2019

Copy link
Contributor

left a comment

LGTM

@@ -507,7 +511,8 @@ ynh_check_app_version_changed () {
then
ynh_print_info --message="Upgrade forced for package check."
else
ynh_die "Up-to-date, nothing to do" 0
ynh_print_info -m "Up-to-date, nothing to do"

This comment has been minimized.

Copy link
@kay0u

kay0u Jul 18, 2019

Contributor

-m => --message= to respect the same syntax of this helper

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

This PR is above all an open question.
I would like to know what do you think of both solutions.

@kay0u

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2019

I would like to know what do you think of both solutions.

Returning a value seems good to me and allows us not to have too much work on all the packages. BUT, check the version of the package before (in python then?) might be better to get a simpler upgrade script.

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

Hum, in both cases we have a value containing the status of the package.
The question would be more,

  • should we exit if the package is up to date, which was supposed to be the behavior until now. (Implies a variable set globally and a modification of all upgrade scripts using that helper.)
  • Or should we tell the script that the package is up to date to let it do what it want with this info.
@kay0u

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

And what if in python the upgrade was not launched if the app is up to date?

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

You mean before the upgrade script even start ?
It could be a solution as well.

@alexAubin

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

And what if in python the upgrade was not launched if the app is up to date?

Yeah, the thing is that maybe not all apps currently implement this mechanism (or don't want to have it enabled)

We could choose to impose this behavior to all apps though... Or have an option in the manifest like upgrade_only_if_version_changes: true or idk

Anyway for the current PR, naively this set -o errtrace scares me because I think it could trigger a lot of unexpected errors from many other situations ? (Though not sure to really understand all the implications)

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

Using set -o errtrace will just propagate set -e into sub shell. It was actually what we would expect from the beginning.
It would be a problem only if sub shell have errors. And so far, we do not allow errors in apps scripts.

@alexAubin

This comment has been minimized.

Copy link
Member

commented Aug 18, 2019

What I'm thinking about is if people did stuff like

if [[ -z "$(grep stuff)" ]] 
then
   ...
fi

then if this grep doesn't find any match, it would make the whole script crash ...? Are we 100% confident that there's no situation like this in the helpers and app script ? (I really don't know 😅 )

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