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] allows to have several domains/path per app #549

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

Conversation

Projects
None yet
3 participants
@Psycojoker
Copy link
Member

Psycojoker commented Oct 2, 2018

The problem

Some apps wants to be able to have several domains/path because they ship several components.

Solution

Allows up to 9 domains per applications. using "domain2/path2" "domain3/path3" ... notation for keys.

I could have made it generic and allows infinite additional domains but that would have made the code way more complex than needed and the case where an app wants 2 domains is already super rare, I can't think of someone wanting more than 3-4 so that should be a good enough marging.

Worst case those people will contact us.

PR Status

Test and working.

BUT, there might be other places in the code where this modification need to be done but I don't know where.

How to test

Edit the config of an app, add "domain2/path2" kind of keys, then do "yunohost app map" and "yunohost app ssowatconf" and see the result.

Validation

  • Principle agreement 0/2 :
  • Quick review 0/1 :
  • Simple test 0/1 :
  • Deep review 0/1 :
@Psycojoker

This comment has been minimized.

Copy link
Member

Psycojoker commented Oct 3, 2018

So, @julienmalik tested it, said it was nearly perfect, it just also needs to handle labels, this raise unexpected new work, todo:

  • handle label[2-9], easy
  • modify change-label behavior, still easy, add an argument like --number
  • modify the admin interface to handle modifications of more than one label: way less easy
  • we probably need to modify the installation part that always ask for a label, might be not needed, the app could want to generate that itself based on label 1
  • modify change_url
  • modify _parse_action_args_in_yunohost_format check at the end
  • modify register_url (and check_url?)

@julienmalik args that since this doesn't break anything we can still have the strategy to merge it then add needed things one by one which would make things easier, I still think we need TODO element 1 (label[2-9]) and explore 4.

@julienmalik

This comment has been minimized.

Copy link
Member

julienmalik commented Oct 3, 2018

Also app_change_url would need modification. Same kind of changes as for change-label

@Psycojoker

This comment has been minimized.

Copy link
Member

Psycojoker commented Oct 4, 2018

Working on the code I realize that:

There are probably other things hiding around.

@zamentur

This comment has been minimized.

Copy link
Contributor

zamentur commented Oct 5, 2018

ssowatconf should be modified too

@Psycojoker

This comment has been minimized.

Copy link
Member

Psycojoker commented Oct 5, 2018

ssowatconf should be modified too

It is already modified, its subcalls are modified.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment