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

Pave the way to migration to php7.3 and following #880

Open
wants to merge 3 commits into
base: stretch-unstable
from

Conversation

@maniackcrudelis
Copy link
Contributor

maniackcrudelis commented Feb 9, 2020

The problem

Php version will change for buster, and so on for next debian release.

Solution

Following #879, prepare the migration by not using a fixed version of php into apps, but the default version set by $YNH_DEFAULT_PHP_VERSION

PR Status

...

How to test

...

Validation

  • Principle agreement 0/2 :
  • Quick review 0/1 :
  • Simple test 0/1 :
  • Deep review 0/1 :
And propagate it as an env variable for apps.
@maniackcrudelis maniackcrudelis requested a review from kay0u Feb 9, 2020
@maniackcrudelis maniackcrudelis mentioned this pull request Feb 9, 2020
0 of 5 tasks complete
@kay0u kay0u mentioned this pull request Feb 10, 2020
0 of 4 tasks complete
Copy link
Member

kay0u left a comment

I think we should add this default php version in the backup script.

env_var["YNH_APP_INSTANCE_NUMBER"] = str(app_instance_nb)

env_dict_remove["YNH_APP_INSTANCE_NUMBER"] = str(app_instance_nb)

env_var["YNH_APP_INSTANCE_NUMBER"] = str(app_instance_nb)

@kay0u kay0u mentioned this pull request Feb 10, 2020
0 of 4 tasks complete
@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

maniackcrudelis commented Feb 10, 2020

I think we should add this default php version in the backup script.

Don't know -_-

@kay0u

This comment has been minimized.

Copy link
Member

kay0u commented Feb 11, 2020

There is an issue in case of backup if this version is not defined. Moreover, I think the PHP help should always call the helper ynh_install_php $YNH_PHP_VERSION to ensure it well installed.

If the default version change, the old default version may be removed, and the backup using the old version crash because his version is not installed.

(Sorry, I miss click the button --')

@kay0u kay0u closed this Feb 11, 2020
@kay0u kay0u reopened this Feb 11, 2020
@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

maniackcrudelis commented Feb 11, 2020

You mean in case of restore ?
If the restore happens on a system where the specific version (or old default) is not installed?
It may indeed doesn't like to not have the service running for its version of php...

As I can see, you did think of this issue into your implementation for nextcloud, https://github.com/YunoHost-Apps/nextcloud_ynh/pull/229/files#diff-68321f788a23b70a33d17888e31bf3efR98

I feel like the best thing to do, into the app scripts, would be to run the helper php instead of restoring the previous config.

@kay0u

This comment has been minimized.

Copy link
Member

kay0u commented Feb 11, 2020

I feel like the best thing to do, into the app scripts, would be to run the helper php instead of restoring the previous config.

The rule is:
When we use the default php version, we don't have to do anything, when we use a specific php version (even if that version is the current default version) the app has to install it via the helper. This way, we should not have any problems during install, upgrade or restore.
We don't need to backup the php fpm file anymore, and during the restore we run the php fpm helper.

Sounds good to me!

@kay0u

This comment has been minimized.

Copy link
Member

kay0u commented Feb 11, 2020

But we still need to add the env var default php version inside the backup script. Do you see any reason to not do it?

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

maniackcrudelis commented Feb 12, 2020

But we still need to add the env var default php version inside the backup script. Do you see any reason to not do it?

What do you mean ?
What I think though about this backup thing is that we should still make a backup, just in case an user would have tweak its config. So it wouldn't be just deleted without backup.

@kay0u

This comment has been minimized.

Copy link
Member

kay0u commented Feb 13, 2020

What do you mean ?

I think we should add YNH_DEFAULT_PHP_VERSION inside backup.py to be able to use php helpers

What I think though about this backup thing is that we should still make a backup, just in case an user would have tweak its config. So it wouldn't be just deleted without backup.

[...]

I feel like the best thing to do, into the app scripts, would be to run the helper php instead of restoring the previous config.

So we make a backup of the php fpm file, but we don't use it during the restore?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.