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

Testing #85

Merged
merged 39 commits into from
May 2, 2020
Merged

Testing #85

merged 39 commits into from
May 2, 2020

Conversation

maniackcrudelis
Copy link

@maniackcrudelis maniackcrudelis commented Jan 31, 2020

Problem

  • No changelog
  • Change is_public was not working as expected
  • No actions and config_panel yet
  • Php is configured with a default config
  • App not up to date
  • wp-fail2ban plugin is becoming a premium plugin with a recurrent ads about going to the premium version. And the main dev of this plugin seems to not care about it.

Solution

  • Add a changelog
  • is_public is either 1 or 0 (not true of false)
  • Add an action for public/private and a config-panel
  • Use ynh_get_scalable_phpfpm to set php according to the ram, cpu and expected usage.
  • Update to 5.4
  • Remove wp-fail2ban and replace it by wp-fail2ban-redux.
  • Fix buster install
  • Add 3 new actions to reset the app

PR Status

  • Code finished.
  • Tested with Package_check.
  • Fix or enhancement tested.
  • Upgrade from last version tested.
  • Can be reviewed and tested.

Validation


Minor decision

  • Upgrade previous version :
  • Code review : Kay0u
  • Approval (LGTM) : Kay0u
  • Approval (LGTM) :
  • CI succeeded :
    Build Status
    When the PR is marked as ready to merge, you have to wait for 3 days before really merging it.

@maniackcrudelis maniackcrudelis marked this pull request as ready for review February 7, 2020 10:46
@maniackcrudelis maniackcrudelis requested a review from a team February 7, 2020 10:46
@maniackcrudelis
Copy link
Author

Ready for review

kay0u
kay0u previously approved these changes Feb 7, 2020
Copy link
Member

@kay0u kay0u left a comment

Choose a reason for hiding this comment

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

LGTM & code review

@lapineige
Copy link
Member

I did my upgrade using yunohost app upgrade wordpress -u https://github.com/YunoHost-Apps/wordpress_ynh yet I'm still in 5.3.2 🤔

@maniackcrudelis
Copy link
Author

@lapineige for wordpress, the upgrade of the package does not upgrade the app itself.
That's for 3 reasons

  • Wordpress is quite a sensitive app regarding security, and so I don't want to upgrade the app only on an upgrade of the package.
  • Wordpress can upgrade itself from its admin panel. Thus, an upgrade from the package could end up with a downgrade, which would probably have some consequences on the database and the plugins.
  • This package add a plugin to auto-upgrade wordpress, and so wordpress is supposed to be up to date.

@lapineige
Copy link
Member

Ok, I was confused by the "Upgrade to 5.4" message in PR.
Yes it updates itself automatically in my case, for minor versions I'm sure, for majors ones I don't remember… And I was not in 5.4.
And upgrade fails… I'll investigate.
Thanks :)

@maniackcrudelis
Copy link
Author

Interested by the result of you investigations, that plugin seems capricious...

@lapineige
Copy link
Member

Well on the web some people said it could be a memory limit, I changed the value in /etc/php/7.0/fpm/conf.d/20-wordpress.ini and restarted php7.0 but nothing changed.

I tried with the us_US version, it worked, and apart from the "what's new" page, everything is still in French 🤔

@maniackcrudelis
Copy link
Author

maniackcrudelis commented Apr 28, 2020

Ok, so actually the plugin did its job, but the upgrade failed ?
I know also that the fr version can fail to upgrade. Don't know why though.

@lapineige
Copy link
Member

I guess so… I didn't check the logs.

@maniackcrudelis
Copy link
Author

Just check my main wordpress, only the upgrade to 5.4 wasn't done.
Maybe as the major version it's not done automatically.

@lapineige
Copy link
Member

The plugin says that it should upgrade major versions too.
It's probably just that major upgrade that failed 🤔

kay0u
kay0u previously approved these changes Apr 28, 2020
Copy link
Member

@kay0u kay0u left a comment

Choose a reason for hiding this comment

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

LGTM & code review 🚀

Thank you for the great work

@kay0u
Copy link
Member

kay0u commented Apr 28, 2020

I did my upgrade using yunohost app upgrade wordpress -u https://github.com/YunoHost-Apps/wordpress_ynh yet I'm still in 5.3.2 thinking

pleas upgrade with:
yunohost app upgrade wordpress -u https://github.com/YunoHost-Apps/wordpress_ynh/tree/testing (with /tree/testing at the end)

@lapineige
Copy link
Member

So it's needed, even if testing is the base branch right now ?

@kay0u
Copy link
Member

kay0u commented Apr 28, 2020

https://github.com/YunoHost/yunohost/blob/dd0f3cd78418f7d1932ae3a122755b3e522e3df5/src/yunohost/app.py#L2106-L2110

It seems that, if no /tree/branchname is provided, then we use master by default :-)

@lapineige
Copy link
Member

It worked :)

@maniackcrudelis
Copy link
Author

Will be merged in 3 days (finally !)

@lapineige
Copy link
Member

Auto-update to 5.4.1 worked, maybe it was only v5.4 that was complicated for some reason…

@kay0u
Copy link
Member

kay0u commented Apr 30, 2020

https://wordpress.org/support/article/configuring-automatic-background-updates/#core-updates

By default, every site has automatic updates enabled for minor core releases and translation files. [...]

Not for major one :-)

@maniackcrudelis
Copy link
Author

Same for me, got my update to 5.4.1 yesterday for one of my wordpress.

@kay0u, the package adds a plugin that is supposed to auto update.
But this plugin seems to work only when it feels like it...

@kay0u
Copy link
Member

kay0u commented Apr 30, 2020

Same for me, got my update to 5.4.1 yesterday for one of my wordpress.

@kay0u, the package adds a plugin that is supposed to auto update.
But this plugin seems to work only when it feels like it...

According to this doc
We can set WP_AUTO_UPDATE_CORE to true (minor by default) to allow major update, but is it something we really want to do?

@maniackcrudelis
Copy link
Author

To auto upgrade yes.
But that's not only the core, but also the plugins and themes. Probably even more dangerous than the core itself.

Anyway, the most important I think is that an user can upgrade as soon as he has a update available. And if it can be done before he even notice it, that's even better.

@lapineige
Copy link
Member

Considering Wordpress (core+plugins+themes) risks regarding security, I'm also for an automatic update of everything, by default. I think it's worth keeping their Wordpress as secure as possible, with the (still limited) risk of breaking their install (while they may have backups) or going into maintenance mode, instead of relying on every admin and user to stay informed about new updates (for every major/minor release, and plugin/theme update) and make the updates in time and taking the risk that their Wordpress or Yunohost server is compromised.

It's easy to disable that plug-in (yet we might advertise that feature in the readme, for those whiling to disable this) if people want to do that by hand.

For my part sometime I see updates for >10 plugins in one day, and several times a week. That would be a pain to maintain everything up-to-date manually.

@lapineige
Copy link
Member

But this plugin seems to work only when it feels like it...

I just did a fresh install : in fact by default it is (now ?) configured not to auto upgrade for major versions.

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.

3 participants