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

[enh] Check before register

- Need to be tested.
- And... probably lack a echo to inform in case of error.
  • Loading branch information...
maniackcrudelis committed May 3, 2017
commit 89856a3f2ba42acf26a0dc22ad979a26b7e6e11a
@@ -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

sudo yunohost domain url-available $domain $path
}

@@ -63,5 +63,6 @@ ynh_webpath_register () {
local app=$1
local domain=$2
local path=$(ynh_normalize_url_path $3)
sudo yunohost app register-url $app $domain $path
ynh_webpath_available $domain $path && \
sudo yunohost app register-url $app $domain $path
}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.