search

YunoHost-Apps / mautrix_signal_ynh

Matrix signal package for YunoHost
GNU Affero General Public License v3.0
4 stars 10 forks source link

Upgrade to v0.2.2 & signald properly & linter #36

Closed Gredin67 closed 2 years ago

Gredin67 commented 2 years ago

Problem

Solution

PR Status

Package_check results

If you have access to App Continuous Integration for packagers you can provide a link to the package_check results like below, replacing '-NUM-' in this link by the PR number and USERNAME by your username on the ci-apps-dev. Or you provide a screenshot or a pastebin of the results

Build Status

Gredin67 commented 2 years ago

!gogogadgetoci

yunohost-bot commented 2 years ago

May the CI gods be with you! Test Badge

MayeulC commented 2 years ago

TO BE CONFIRMED: signald does not store user data, which are stored in mautrix_signal DB

That assumption is wrong: you can check it with signaldctl.

I like the idea, but that feels like a kludge. I think #34 is a better approach. It was stalled for a bit due to a broken CI, but let's see if I can make it work.

Gredin67 commented 2 years ago

I'm in contact with YunoHost maintainers to try to solve the helper issue.

MayeulC commented 2 years ago

34 seems to be in good shape now. It's a bit harder to maintain as checksums have to be updated, but on the other hand, I really like the fact that packages are pinned to a verified version, so I think we can go with it for now.

I'm going to test on my server, then merge this into testing, and finally upgrade to 0.2.1.

alexAubin commented 2 years ago

Pending fix in core for the upgrade issue: https://github.com/YunoHost/yunohost/pull/1407

alexAubin commented 2 years ago

(Fix in the core was released in stable a couple days ago)

Gredin67 commented 2 years ago

!gogogadgetoci

yunohost-bot commented 2 years ago

:carousel_horse: Test Badge

Gredin67 commented 2 years ago

!testme

yunohost-bot commented 2 years ago

:rocket: Test Badge

MayeulC commented 2 years ago

I would be much, much more comfortable reviewing, testing and merging one item at a time :sweat_smile:

  1. upgrade fix
  2. linting
  3. mautrix_signal upgrade

Especially since the commit log isn't particularly clean or expressive.

However, I agree with merging them all at once to master.

MayeulC commented 2 years ago

I can try to address some of these comments myself if you want, but I didn't really want to change some of that without your consent.

Gredin67 commented 2 years ago

  1. upgrading the source file does not require review, just CI test

  2. There was a PR about linting which was opened for long time and nobody reviewed, so I improved it and merged

  3. the mautrix_signal upgrade has been fixed in the core, not here --> would be happy if someone confirms that signald actually upgrades now

This will be squashed, and nobody cares about a Yunohost package history. That's why I don't care about messing up the history.

Feel free to review this one or split it and review if you think that makes sense ;) I will check you review and take the necessary changes