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

Nouveau helper ynh_webpath_available et ynh_webpath_register #235

Merged
merged 7 commits into from May 6, 2017

Conversation

Projects
None yet
6 participants
@maniackcrudelis
Copy link
Contributor

commented Jan 22, 2017

Simplement un wrapper de yunohost app checkurl.

Peut-être une occasion de lui donner un autre nom plus parlant.
Il me semble justement qu'il en était question.

@zamentur

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2017

Le problème c'est le nom et le fait que ça modifie le système. ynh_check_url devrait être une sorte de fonction "pure" (pas d'action sur le système juste du contrôle).

Il faudrait une autre fonction ynh_reserve_url pour réserver l'url. (et qui pourrait éventuellement inclure le contrôle pour des raisons pratiques).

# usage: ynh_path_validity $domain_and_path
# | arg: domain_and_path - complete path to check
ynh_path_validity () {
sudo yunohost app checkurl $1 -a $app

This comment has been minimized.

Copy link
@julienmalik

julienmalik Jan 23, 2017

Member

it would be better to not call the moulinette at all, and instead rewrite it in pure bash.

This comment has been minimized.

Copy link
@maniackcrudelis

maniackcrudelis Jan 23, 2017

Author Contributor

Si je comprend bien, checkurl est ici:
https://github.com/YunoHost/yunohost/blob/unstable/src/yunohost/app.py#L888-L935

Un bon gros morceau, qui va pas être simple à comprendre...

This comment has been minimized.

Copy link
@maniackcrudelis

maniackcrudelis Jan 23, 2017

Author Contributor

Bon globalement je comprend pas les détails... (Et ça m'agace un peu je dois bien le confesser...)

Dans le principe, si je ne me trompe pas il y 2 aspect (J'ai repris la discussion épique de Bram et Maxime à ce sujet)

  • D'une part, le nouveau path ne doit pas être existant dans un path déjà utilisé. Impossible de créer domain.tld/path si j'ai déjà domain.tld/path_to_app.
  • D'autre part, aucun path déjà utilisé ne doit être présent dans le nouveau. Impossible de créer domain.tld/path si j'ai déjà domain.tld/pat.

Si on est d'accord avec ça, je propose ceci:

  • yunohost app map | grep domain/path pour chercher le premier cas.
  • Et une boucle sur la sortie de yunohost app map pour faire un grep dans l'autre sens
    echo domain/path | grep line
    En tronquant à partir du : et de l'éventuel / final.

This comment has been minimized.

Copy link
@Psycojoker

Psycojoker Jan 25, 2017

Member

Je ne pense pas que recoder cela en bash soit une bonne idée, les opérations faites dans cette fonction sont non trivial à porter en bash. Ca implique aussi de recoder "app_map" et vous ne voulez pas avoir à faire ça en bash.

This comment has been minimized.

Copy link
@maniackcrudelis

maniackcrudelis Jan 25, 2017

Author Contributor

lol !
Je veux bien te croire.

Toutefois j'avais compris que ce qui est problématique ce sont les commandes YunoHost qui ne servent qu'au packaging. Tel que yunohost app checkurl qui ne sert pas à l'administration.
En revanche yunohost app map sert à l'administration en CLI.

Ce faisant, ce que j'en comprend c'est qu'il n'est pas nécessaire de recoder app map
Dans ce cas, c'est simple, comme indiqué dans mon précédent commentaire.

This comment has been minimized.

Copy link
@Psycojoker

Psycojoker Jan 25, 2017

Member

Dans ce cas je vois pas trop l'intérêt de re-coder ça en bash. Surtout qu'on va probablement vouloir à terme faire un check avant de lancer le script d'installer dans l'admin web pour vérifier si un path est disponible et donc utiliser ça.

This comment has been minimized.

Copy link
@maniackcrudelis

maniackcrudelis Jan 25, 2017

Author Contributor

Sinon on reste sur le helper tel qu'il est, juste pour l'abstraction de la commande yunohost CLI.
Ça laisse le temps de l'adoption, et après on pourra facilement en modifier le contenu selon la décision prise.

@Psycojoker Psycojoker added this to the 2.6.x milestone Feb 3, 2017

#
# usage: ynh_path_validity $domain_and_path
# | arg: domain_and_path - complete path to check
ynh_path_validity () {

This comment has been minimized.

Copy link
@alexAubin

alexAubin Feb 27, 2017

Member

Imho this name is confusing :

  • path alone is ambiguous and might be interpreted as a filesystem path (like /etc/toto)
  • validity might be interpreted as "syntaxically valid"

I would suggest something like ynh_url_path_available

This comment has been minimized.

Copy link
@alexAubin

alexAubin Feb 27, 2017

Member

Actually it's not clear to me if this helper / function has side-effect or not. From the name of the helper, I would think it doesn't and that I need to run some sort of ynh_url_path_register if the path was found to be available. But from the code of checkurl it's not clear. Though it looks like if no app name is given, it doesn't have side-effects ?

Btw does this helper works in $app is not defined ?

This comment has been minimized.

Copy link
@maniackcrudelis

maniackcrudelis Feb 27, 2017

Author Contributor

I think you're better than me to read this code.
https://github.com/YunoHost/yunohost/blob/unstable/src/yunohost/app.py#L888-L935

For me, it's just a control without side-effect. But, I didn't read it again.

This comment has been minimized.

Copy link
@alexAubin

alexAubin Mar 5, 2017

Member

To me it looks like if app is defined, then some settings of the app are changed/set : https://github.com/YunoHost/yunohost/blob/unstable/src/yunohost/app.py#L947-L949

So there are side-effects, but only if app is defined. And as was trying to point out in the previous comment, your helper implicitly require $app (in bash this time) is defined. So bottom line is : if you remove the -a $app, then everything should be fine 😄 !

As a side note though, it's not really cool that the python throws an exception if the path is not available. Not sure if this is considered as a side effect... But that's basically the same kind of situation which led to the dirty no_log helper :/.. We should really refactor these functions :/

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2017

So, what we doing with this helper?
Do we keep it or not?

Is it really useful?

@alexAubin

This comment has been minimized.

Copy link
Member

commented Mar 14, 2017

I think it's clearly an important and useful helper. From what I understand there are two main items remaining :

  • Clarify how $app should be defined ? Should it be a global variable ? Should it be passed as an argument ?
  • I created #267 in order to fix the semantic of this. We can either merge this PR already then update this helper when 267 will be merged, OR we wait for 267 to be merged.
#
# example: ynh_webpath_available $domain$path
#
# usage: ynh_webpath_available $domain_and_path

This comment has been minimized.

Copy link
@alexAubin

alexAubin Mar 26, 2017

Member

Following this remark from Bram, I think we should avoid merging the domain and path, to anticipate the upcoming urlavailable helper which take domain and path as separate arguments (make stuff a lot easier).

So we could simply have ynh_webpath_available $domain $path instead.

@alexAubin alexAubin changed the title Nouveau helper ynh_path_validity Nouveau helper ynh_webpath_available Mar 26, 2017

@alexAubin alexAubin modified the milestones: 2.6.x, 2.7.x Mar 27, 2017

maniackcrudelis and others added some commits Jan 22, 2017

Nouveau helper ynh_path_validity
Simplement un wrapper de yunohost app checkurl.

Peut-être une occasion de lui donner un autre nom plus parlant.
Il me semble justement qu'il en était question.
Conflicts:
	data/helpers.d/network

@alexAubin alexAubin force-pushed the ynh_path_validity branch from 0527e99 to c49ee34 Mar 29, 2017

@alexAubin

This comment has been minimized.

Copy link
Member

commented Mar 29, 2017

(Rebased to unstable to have ynh_normalize_url available)

@alexAubin
Copy link
Member

left a comment

Tested, works as expected

@alexAubin

This comment has been minimized.

Copy link
Member

commented Apr 16, 2017

So we now have the "domain url-available" action with no side-effect merged (c.f. #267). Shall we have an aditional helper for the new "app register-url" ?

@alexAubin alexAubin changed the title Nouveau helper ynh_webpath_available Nouveau helper ynh_webpath_available et ynh_webpath_register Apr 25, 2017

@alexAubin

This comment has been minimized.

Copy link
Member

commented Apr 25, 2017

I added an (untested) ynh_webpath_register helper that calls the new yunohost app register-url command to register/book a path for a specific app.

The command will fail (raise an exception) if the path is already taken. So depending of what how you want to handle things, you either use this alone and expect/hope the path is not taken (#yolo) OR you check with ynh_webpath_available before, and properly handle the case where it's not in your install script.

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2017

What do you think about simply add a call to ynh_webpath_available in ynh_webpath_register?
And handle the fail case into this helper.

@alexAubin

This comment has been minimized.

Copy link
Member

commented Apr 30, 2017

What do you think about simply add a call to ynh_webpath_available in ynh_webpath_register?
And handle the fail case into this helper.

My point was that handling the case where the path you're trying to register is unavailable is app-dependent I guess. For instance, if you install wordpress and the user told "I want to install wordpress installed on domain yolo.com with path /swag", you can try to register it and it'll fail, but :

  • either the packager just calls ynh_webpath_register, assuming it's available, otherwise it'll crash and that's all because there's no fallback anyway
  • either the packager has a fallback strategy like trying ynh_wepath_register yolo.com/swag2, then /swag3, ...

But now that I think about it, the issue should probably be solved in the core : in the same way that we check that the domain specified by the user exists (I think ?), we might/should check that the domain/path is available, before even attempting to run the install script ?

Bottom line, I think if the current implementation of ynh_webpath_register works (as said, I didn't test it 😛 ), I think we're all good with this PR and we should open an issue about implementing the availability check before running the install script 😛

@alexAubin

This comment has been minimized.

Copy link
Member

commented May 2, 2017

I opened a ticket regarding my previous point : https://dev.yunohost.org/issues/909

Unless there's opposition, I propose to merge these helpers as they are right now, since they fulfill the initial specification.

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2017

I rather disagree with your idea to change /swag in /swag2, simply because if an user wants to use /swag, maybe he doesn't want to have /swag2 lastly. Especially if his app is public.

But, your idea to check that before the installation, like the domain, sounds good.

If you're ok with that, I'm adding a last (I hope) commit to add a check with ynh_webpath_available in ynh_webpath_register

[enh] Check before register
- Need to be tested.
- And... probably lack a echo to inform in case of error.
@alexAubin

This comment has been minimized.

Copy link
Member

commented May 3, 2017

I rather disagree with your idea to change /swag in /swag2, simply because if an user wants to use /swag, maybe he doesn't want to have /swag2 lastly. Especially if his app is public.

Yes, I was just trying to imagine what could be a strategy, but obviously this is not a really good idea. (It would be like installing the app on another domain than the one initially requested by the user)

If you're ok with that, I'm adding a last (I hope) commit to add a check with ynh_webpath_available in ynh_webpath_register

Thing is, ynh_webpath_register already checks that the url is available (inside the code of "yunohost tools url-available") and throw an error if it's not (just like the old checkurl before). So except if you have something you'd like to do in this helper in case the URL is not available, which is not "throw an error", I don't see what you'd like to achieve with this

@@ -47,7 +47,7 @@ ynh_find_port () {
# | arg: path - the web path to check the availability of
ynh_webpath_available () {
local domain=$1
local path=$(ynh_normalize_url_path $2)
local path=$2

This comment has been minimized.

Copy link
@alexAubin

alexAubin May 3, 2017

Member

Uh, why not normalize the url path ?

This comment has been minimized.

Copy link
@maniackcrudelis

maniackcrudelis May 3, 2017

Author Contributor

Because, if it seems a good idea to correct the path in ynh_webpath_register (because we gonna reserve this path).
I think when you simply check the path, you should use the real path like it's stored or defined.

And, in my commit, ynh_webpath_available was called just after a call to ynh_normalize_url_path

But, what do you mean by "ynh_webpath_register already checks that the url is available"?
register-url uses url-available before?

In this case, my commit is useless.

This comment has been minimized.

Copy link
@alexAubin

alexAubin May 3, 2017

Member

But, what do you mean by "ynh_webpath_register already checks that the url is available"?
register-url uses url-available before?

Yes (here)

I think when you simply check the path, you should use the real path like it's stored or defined.

Uh I don't know why you want to use the "real" path when checking availability and not when registering :/ (with or without calling normalize, they're the same anyway, they're just not necessarily formatted the same way ?). Anyway they're renormalized inside the python code so that doesn't really matter ... In fact we should probably normalize the path received from the user/manifest before passing it to the install script.

This comment has been minimized.

Copy link
@maniackcrudelis

maniackcrudelis May 3, 2017

Author Contributor

In my point of vue, ynh_normalize_url_path should be used only before the registering of the path, in the install script.
Because, this helper is designed only to prevent a foolish error. Just to prevent to crash the script for a small error.

But, this path should be fixed immediately, that is why I think we should use this helper only at the beginning of the install script.

This comment has been minimized.

Copy link
@alexAubin

alexAubin May 3, 2017

Member

But, this path should be fixed immediately, that is why I think we should use this helper only at the beginning of the install script.

Okay, agreed on that. If you like, we can remove ynh_normalize_url_path from both helpers so that everything is consistent ;)

This comment has been minimized.

Copy link
@maniackcrudelis

maniackcrudelis May 3, 2017

Author Contributor

I don't know, because if we keep a call to ynh_normalize_url_path in ynh_webpath_register, it's not needed to add it in the install script.
So, I think it's more simple for all the packagers like that.

And it avoids a forgot.

This comment has been minimized.

Copy link
@alexAubin

alexAubin May 4, 2017

Member

After discussing on the chat, we agreed that :

  • the normalizing of the path should be done before the install script ideally, therefore we should remove both ynh_normalize_url_path from these helpers
  • the availability check should be done before even launching the install script

Both these points are implemented in #304

Dismissing review because outdated. Feel free to update your position on the PR !

@alexAubin
Copy link
Member

left a comment

Re-approving. If we have @maniackcrudelis 's greenlight, I suggest merging this in a few days.

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2017

You have, even if I can't approve with github :)

@alexAubin alexAubin merged commit 3571747 into unstable May 6, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@alexAubin alexAubin deleted the ynh_path_validity branch May 6, 2017

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