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

Update to respect latest YEP #14

Merged
merged 40 commits into from Feb 20, 2017
Merged

Conversation

polytan02
Copy link
Contributor

Hi,

I updated the scripts so that :

  • Install, upgrade, remove work on most use case
  • backup and restore now exist
  • Sources are downloaded isntead of being in the depot

I tested the app, it installs fine, I can access the hubzilla.
I haven't done much further tests on the app itself.

One important things : haven't seen any difference by modfying the php.ini pcntl_exec value, so currently it is not modified.
If you find it is important, I can help in the scripts but please make sure that upgrade and remove scripts bring php.ini AS PER BEFORE HUBZILLA_YNH modifies it

Oh, and please could you rename the depot in hubzilla_ynh (this helps people finding your project as ALL yunohost apps end in _ynh :) )

@polytan02
Copy link
Contributor Author

Not sure what you mean :)

Do you want to include the modifications I propose ?

@anaqreon
Copy link
Member

I was confused about the target repo for the updates, nevermind. This is a lot of work; merci! I have not tested this myself but I've looked through most of the code. I like the smaller repo size by downloading the Hubzilla source code on-the-fly. Here are my comments so far:

  1. The exec() PHP function must be enabled for Hubzilla to work properly. It may not prevent it from being installed and accounts created, etc, but Hubzilla will be broken in other ways.
  2. Do we really want to restore the php.ini file if Hubzilla is uninstalled? What if, after you install Hubzilla, another app is installed that also needs a change that was made either for Hubzilla or after Hubzilla was installed? In that case, reverting the php.ini file will break the other app.
  3. For the purposes of the YunoHost app, I think copying the official addons into /addon might be sufficient, but the proper way is to copy it into /extend/addon/official/ and then run the util/update_addon_repo official utility to create the symlinks in /addon.
  4. I will create a new repo with the name "hubzilla_ynh" using this pull request as the initial state. We can continue improving it until it is ready to replace the current repo, at which time I will delete the original repo.

@anaqreon
Copy link
Member

@polytan02
Copy link
Contributor Author

I don't think you need to do a new repo, can't you simply rename this one ?

Regarding your points :

  1. & 2) if you look at the code for install, upgrade and remove, I prepared everything for the app to adjust the php.ini : all it needs is remove the #
    I really believe php.ini should not be modified and as far as I know, only hubzilla does it, so yes, we can put the php.ini as it was "before" :)
  2. no problem, it seems even better to me : the scripts can be modifed to do all of this (and still download the files on the fly)

@anaqreon
Copy link
Member

Unfortunately I transferred ownership to YunoHost-Apps and now I cannot delete the new "hubzilla_ynh" repo in order to allow me rename the original "hubzilla-yunohost" repo :-/ I'll ask @abeudin to remove it or give me permission to.

@anaqreon
Copy link
Member

I really believe php.ini should not be modified and as far as I know, only hubzilla does it

This Hubzilla YunoHost app seems to be workable, but Hubzilla is, according to their documentation, "more than a simple web application. It is a complex communications system which more closely resembles an email server than a web server." But, YunoHost has an email server app, so there you go.

Doesn't the Nextcloud app require changes to the php.ini file in order to increase the upload file size limits?

@polytan02
Copy link
Contributor Author

@anaqreon My last commit should adjust nginx to bigger upload.
There is no need to modify the php.ini for size uploads : we modify the nginx hubzilla local conf (stored in /etc/nginx/conf.d/domain.tld/hubzilla.conf and we also modify the php-fpm hubzilla local conf (stored in /etc/php/fpm/pool.d/hubzilla.conf)

If you want, all modifications required for hubzilla are in dedicated files and don't impact the other apps.

Is it OK for you to merge all my commits into the official rep ?

@polytan02
Copy link
Contributor Author

the the depot name, I will ask @maniackcrudelis to delete Yunohost-Apps/hubzilla_ynh and then rename Yunohost-Apps/hubzilla-yunohost in _ynh

@polytan02
Copy link
Contributor Author

@anaqreon I have done some extra cleaning and also modified the manifest to let the admin choose to modify the php.ini or not. And at least they cannot say they were not aware :)

@maniackcrudelis
Copy link
Contributor

Do we really want to restore the php.ini file if Hubzilla is uninstalled? What if, after you install Hubzilla, another app is installed that also needs a change that was made either for Hubzilla or after Hubzilla was installed? In that case, reverting the php.ini file will break the other app.

You should never modify the global php.ini
If you need a special configuration for your app, use a dedicated php.ini file.
And put it in this directory: /etc/php5/fpm/conf.d/

For hubzilla_ynh and hubzilla-yunohost, I'll restore your access on both of them.

@anaqreon
Copy link
Member

I've renamed the repo to "hubzilla_ynh" and now I'm reviewing your other commits and comments so I can understand them. I did not know you could make local modifications to the PHP settings on a per-app basis, so thanks for that.

@anaqreon anaqreon merged commit 76673be into YunoHost-Apps:master Feb 20, 2017
@anaqreon
Copy link
Member

I merged the changes so we could stay synchronized more cleanly, but there are still some issues:

  1. The ability to have a non-public hub was reintroduced into the manifest.json file. I am not 100% certain, but this might break Hubzilla functionality; hubs communicate via HTTP GET/POST requests in the background, and if these requests get "SSOed" it will probably break the hub.
  2. Like I explained before, the exec() function is a requirement. I like the idea of making sure the administrator knows what they are doing by installing Hubzilla, but it cannot be a choice. Line 107 of the install script just echos that php.ini was not modified. Can we remove this option from manifest.json (just issue a warning) and then enable exec() somehow in /etc/php5/fpm/pool.d/$app.conf like in the lines immediately below (110-115)?
  3. The package is actually small now because the sources were removed, but the git history still has the ~150MB of data. We should consider a fresh repo at the expense of history.

Thanks to both of you for contributing so much to making this app better. I am a big believer in Hubzilla and so I know it is worthy of your efforts; it shares a lot of the same vision and purpose that YunoHost does, and it supports an impressive set of applications for the decentralized web.

@polytan02
Copy link
Contributor Author

We may have to create a new repo indeed :(

Have you tried things like that : https://help.github.com/articles/removing-files-from-a-repository-s-history/

https://help.github.com/articles/removing-sensitive-data-from-a-repository/

Regarding the exec() function, we can dig but I think it is a system wide function, it is important to keep the line in the manifest. Also, today there is no possibility to put a notice before the app is installed.

Copy link
Contributor Author

@polytan02 polytan02 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you do an upgrade of an existing install, the run_exec varaible doesn't exist, so will never be added : or you remove and reinstall the app, or we force it by putting yes by default and adding the parameter

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

Successfully merging this pull request may close these issues.

None yet

3 participants