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

[enh] Rebuild applist system and make it future proof #778

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

Conversation

@alexAubin
Copy link
Member

commented Aug 17, 2019

The problem

So ... been ranting about the applist system for a long time. Especially because :

  • The code is still a mess even though it has been rewritten ~2 years ago. And there's still a bunch of legacy code lying around.
  • The ability to add / remove additional list is a big YAGNI leading to unecessary complex code, dozens of strings to internationalize, and regularly bugs (though now fixed)
  • When fixing the bugs, one regularly had to edit the json file which is a mess to edit manually if you're not used to it
  • Since I'm in the process of implementing the app categories, for this we need to change the format of apps.json ... which leads to yet again another migration to implement. And I'm pretty sure than in less than 6 months we will want to change the format again

Solution

  • Burn the existing code and rework everything
  • Use a yaml for conf because it's much more readable and easy to edit manually
  • 99.9% of our user base don't need to add custom list, so let's drop the API about adding/removing/listing the appslists. For power user than want to add additional lists, we'll simply tell them to edit the config file manually, which just isn't complex if it's a yaml file instead of json where you just need to put a name and a url. (It's not difficult to automatize either, you can just append the appropriate text to the conf file with an echo)
  • Make the system future proof, meaning using a version number for the appslist API. So instead of fetching app.yunohost.org/apps.json, we fetch app.yunohost.org/default/v{VERSION}/apps.json where :
    • default is the name of the list ... in case in future we want to support additional list (e.g. dev for special apps, or non-free though this is just an example, not really pushing for it...) with the same domain app.yunohost.org
    • {VERSION} is a number like 1, 2, 3, ... hardcoded in the code. Later in the future if we want to change the apps.json format. we just update the version in the code, expose the new json on /default/v{NEW_VERSION}/apps.json and the yunohost instance shall automatically see that the previous cache is outdated and update it.

PR Status

Tested on my machine with the unit tests provided in the PR

How to test

Read and run the unit tests ¯\_(ツ)_/¯

Validation

  • Principle agreement 0/2 :
  • Quick review 0/1 :
  • Simple test 0/1 :
  • Deep review 0/1 :
@decentral1se
Copy link
Member

left a comment

Some more half-informed, passing by, at least another pair of eyeballs reviews 👍

@decentral1se

This comment has been minimized.

Copy link
Member

commented Aug 24, 2019

99.9% of our user base don't need to add custom list, so let's drop the API about adding/removing/listing the appslists.

+1

using a version number for the appslist API.

+1

alexAubin and others added 2 commits Aug 24, 2019
Update src/yunohost/data_migrations/0010_migrate_to_apps_json.py
Co-Authored-By: decentral1se <lukewm@riseup.net>
except YunohostError as e:
logger.error(m18n.n('tools_update_failed_to_app_fetchlist'), error=e)
logger.error(str(e))

This comment has been minimized.

Copy link
@Psycojoker

Psycojoker Aug 25, 2019

Member

We might want a better error here for later 😅

@alexAubin alexAubin added this to the 3.8.x milestone Sep 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.