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

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

Merged
merged 38 commits into from
Jan 25, 2022
Merged

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

merged 38 commits into from
Jan 25, 2022

Conversation

Gredin67
Copy link
Collaborator

@Gredin67 Gredin67 commented Jan 3, 2022

Problem

  • helper does not upgrade
  • code is messy

Solution

PR Status

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

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
Copy link
Collaborator Author

Gredin67 commented Jan 3, 2022

!gogogadgetoci

@yunohost-bot
Copy link
Contributor

May the CI gods be with you!
Test Badge

@Gredin67 Gredin67 linked an issue Jan 3, 2022 that may be closed by this pull request
@MayeulC
Copy link
Collaborator

MayeulC commented Jan 4, 2022

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
Copy link
Collaborator Author

Gredin67 commented Jan 5, 2022

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

@MayeulC
Copy link
Collaborator

MayeulC commented Jan 5, 2022

#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.

scripts/upgrade Outdated Show resolved Hide resolved
@alexAubin
Copy link
Member

Pending fix in core for the upgrade issue: YunoHost/yunohost#1407

@alexAubin
Copy link
Member

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

* Update README.md

* prepare for add to app list

* restore signald user data folder

* clean-up and sync whatsapp_ynh

* Update app.src

* Update manifest.json

Co-authored-by: ericgaspar <junk.eg@free.fr>
Co-authored-by: Yunohost-Bot <>
@Gredin67 Gredin67 changed the title Upgrade to v0.2.1 & upgrade signald properly Upgrade to v0.2.2 & upgrade signald properly Jan 18, 2022
@Gredin67 Gredin67 changed the title Upgrade to v0.2.2 & upgrade signald properly Upgrade to v0.2.2 & signald properly & linter Jan 18, 2022
@Gredin67
Copy link
Collaborator Author

!gogogadgetoci

@yunohost-bot
Copy link
Contributor

🎠
Test Badge

@Gredin67
Copy link
Collaborator Author

!testme

@yunohost-bot
Copy link
Contributor

🚀
Test Badge

@MayeulC
Copy link
Collaborator

MayeulC commented Jan 20, 2022

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
Copy link
Collaborator Author

  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

Gredin67 and others added 2 commits January 20, 2022 16:19
Co-authored-by: Mayeul Cantan <mayeul.cantan@gmail.com>
Copy link
Collaborator Author

@Gredin67 Gredin67 left a comment

Choose a reason for hiding this comment

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

I integrated most things, closed some that I think are not useful or should belong to another PR. Things that are still open require more discussion or to do some modifications.

doc/DISCLAIMER.md Show resolved Hide resolved
doc/DISCLAIMER.md Outdated Show resolved Hide resolved
* Par défaut, seules les conversations avec des messages très récents seront mises-en-miroir
* Acceptez les invitations aux salons

#### Enregistrer la passerelle comme appareil principal
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

can you check and do it?

manifest.json Outdated
Comment on lines 54 to 66
{
"name": "bot_synapse_adm",
"type": "boolean",
"ask": {
"en": "Give the WhatsApp bot administrator rights to the Synapse instance?",
"fr": "Donner au robot WhatsApp des droits administrateur à l'instance Synapse ?"
},
"help": {
"en": "If true, the bot can group WhatsApp chats in a Matrix space. Not required if you set up Synapse so that non-admins are authorized to create spaces.",
"fr": "Si true, le robot groupera les conversations WhatsApp dans un espace Matrix. Pas nécessaire si vous avez réglé Synapse pour qu'il autorise les non-admin à créer des espaces."
},
"default": true
},
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

again, to be checked. Yep for the typo

manifest.json Outdated Show resolved Hide resolved
manifest.json Show resolved Hide resolved
scripts/_common.sh Show resolved Hide resolved
@Gredin67 Gredin67 merged commit 66c49f0 into master Jan 25, 2022
@Gredin67 Gredin67 deleted the testing branch February 2, 2022 09:50
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.

Investigate signald upgrade
5 participants