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

[fix] Split checkurl into two functions : availability + booking #267

Merged
merged 12 commits into from Apr 16, 2017

Conversation

Projects
None yet
5 participants
@alexAubin
Copy link
Member

commented Mar 6, 2017

Status

Related to these discussions / tickets

Problem

  • app checkurl is not semantic because 'check' is pretty vague, and this function has sideeffects if the app parameter is given
  • @maniackcrudelis wants to provide a bash helper around it but many things either can't be done or are ambiguous because of the current implemntation

Solution

  • This PR split this function in two, one is tools urlavailable which returns { "available" : "Yes" } (or no) if domain.tld/path can be used. (We can easily build a bash helper around it)
  • The other one is app bookurl which set the appropriate settings for an app to be considered installed on a given url.
  • app checkurl gets a depreciation warning

This is still not fully tested. I need to setup and write some propre unit tests with some dummy app. (The bookurl part can only be tested during an app install :/). Writing the tests will be pretty great for future development though ;) Done.

This is also related to #185 because we might want to point user to 'change_url' when user are trying to use 'bookurl' on an already installed app.

@alexAubin alexAubin changed the title Split checkurl into two functions : availability + booking [fix] Split checkurl into two functions : availability + booking Mar 6, 2017

@Psycojoker
Copy link
Member

left a comment

The code is way better like that, thanks :)

### app_bookurl()
bookurl:
action_help: Book a web path for a given app
api: GET /tools/bookurl

This comment has been minimized.

Copy link
@Psycojoker

Psycojoker Mar 6, 2017

Member

Should probably be a POST/PUT request to create a new entry according to Rest semantic.

This comment has been minimized.

Copy link
@alexAubin

alexAubin Mar 11, 2017

Author Member

Indeed, will fix that !

app_setting(app, 'domain', value=domain)
app_setting(app, 'path', value=path)


def app_checkurl(auth, url, app=None):

This comment has been minimized.

Copy link
@Psycojoker

Psycojoker Mar 6, 2017

Member

Shoudln't checkurl now uses those 2 new functions?

This comment has been minimized.

Copy link
@alexAubin

alexAubin Mar 11, 2017

Author Member

Hm, I dunno yet, I don't realize how risky it is to change it.. But maybe if the two new functions are well tested and if we keep an eye on the app CI on testing/unstable it's okay to change it

This comment has been minimized.

Copy link
@alexAubin

alexAubin Mar 14, 2017

Author Member

This is a remaining item. Still not sure what to do about this 😕

This comment has been minimized.

Copy link
@Psycojoker

Psycojoker Mar 14, 2017

Member

Not sure either 😕

This comment has been minimized.

Copy link
@alexAubin

alexAubin Mar 26, 2017

Author Member

So, I'd rather avoid to change app_checkurl to be sure to avoid breaking things. Using the new functions would not be trivial because a split of the domain/path would still be needed and it's not clear how I should handle the "app" argument.

apps_map = app_map(raw=True)

# Loop through all apps to check if path is taken by one of them
available = "Yes"

This comment has been minimized.

Copy link
@Psycojoker

Psycojoker Mar 6, 2017

Member

Shouldn't we uses a boolean value? This is json serializable and will be easier to uses in the admin interface.

This comment has been minimized.

Copy link
@alexAubin

alexAubin Mar 11, 2017

Author Member

Yes indeed

# Loop through apps
for p, a in apps_map[domain].items():
if path == p:
available = "No"

This comment has been minimized.

Copy link
@Psycojoker

Psycojoker Mar 6, 2017

Member

We should return here since we have our answer.

# We also don't want conflicts with other apps starting with
# same name
elif path.startswith(p) or p.startswith(path):
available = "No"

This comment has been minimized.

Copy link
@Psycojoker

Psycojoker Mar 6, 2017

Member

Same regarding return.

url = url.rstrip("/")

# Check url contains at least a slash
if "/" not in url:

This comment has been minimized.

Copy link
@Psycojoker

Psycojoker Mar 6, 2017

Member

This will fails if the url is domain.com/ because we rstrip before.

raise MoulinetteError(errno.EINVAL, m18n.n('invalid_url_format'))

# Now we extract the domain and uri/path
domain = url[:url.index('/')]

This comment has been minimized.

Copy link
@Psycojoker

Psycojoker Mar 6, 2017

Member

I think that url.split("/")[0] would be nicer and also safer (since string.index will raise in the substring is not present).

Also this won't behave as expected if the domain is domain.com/ will with string.split it will.

# Now we extract the domain and uri/path
domain = url[:url.index('/')]

path = url[url.index('/'):]

This comment has been minimized.

Copy link
@Psycojoker

Psycojoker Mar 6, 2017

Member

Same comment than before.

However, here we will need something like:

if "/" in url:
    path = url.split("/", 1)[1]
else:
    path = "/"
if not path.endswith('/'):
path = path + '/'

return domain, path

This comment has been minimized.

Copy link
@Psycojoker

Psycojoker Mar 6, 2017

Member

A more general comment for this function is: why the hell do we merge domain+path when calling YunoHost function to split them afterward? This doesn't make any sens and make more work.

I never saw a location where we have domain+path already merged, we always merge them then split them.

I think that we should change the API and simply expect the domain and path and not their combination, this will simplify the code a lot :)

This comment has been minimized.

Copy link
@alexAubin

alexAubin Mar 11, 2017

Author Member

Hm yes indeed, will have a look into that

if url.startswith("https://"):
url = url[len("https://"):]
elif url.startswith("http://"):
url = url[len("http://"):]

This comment has been minimized.

Copy link
@Psycojoker

Psycojoker Mar 6, 2017

Member

I don't know if using url.lstrip("http://") and url.lstrip("https://") wouldn't make this code better.

This comment has been minimized.

Copy link
@alexAubin

alexAubin Mar 11, 2017

Author Member

Unfortunately a.lstrip(b) removes all characters from b that are at the beginning of a, so for instance "https://hello".lstrip("https://") would return "ello" 😕

This comment has been minimized.

Copy link
@Psycojoker

Psycojoker Mar 11, 2017

Member

Damn, that's stupid, I though it was treating the whole string as a single unit :(

@M5oul M5oul added this to the 2.6.x milestone Mar 7, 2017

@alexAubin

This comment has been minimized.

Copy link
Member Author

commented Mar 12, 2017

Added unit tests. You'll need to git clone https://github.com/YunoHost/test_apps ./apps inside the tests folder.

@polytan02

This comment has been minimized.

Copy link

commented Mar 12, 2017

Do we use this tester in the samme way as package_linter ?

@alexAubin

This comment has been minimized.

Copy link
Member Author

commented Mar 14, 2017

Do we use this tester in the samme way as package_linter ?

Not sure I get the question. What do you mean by "the samme way as package_linter" ?

@alexAubin

This comment has been minimized.

Copy link
Member Author

commented Mar 15, 2017

Maybe we should address https://dev.yunohost.org/issues/688 also with this PR and adapt the corresponding messages so they are meaningful

@Psycojoker

This comment has been minimized.

Copy link
Member

commented Mar 17, 2017

I really prefer that we avoid putting too much things in a PR and making it too hard to merge. See refactoring of fetchlist as a reference.

@alexAubin

This comment has been minimized.

Copy link
Member Author

commented Mar 18, 2017

I really prefer that we avoid putting too much things in a PR and making it too hard to merge. See refactoring of fetchlist as a reference.

Okay, agreed, we can make a small PR later

@@ -1351,6 +1368,21 @@ tools:
help: Show private data (domain, IP)
action: store_true

### tools_urlavailable()
urlavailable:

This comment has been minimized.

Copy link
@zamentur

zamentur Mar 31, 2017

Contributor

I am for the use of "yunohost tools url-available" .
Dash is used everywhere but in app cmd. We should fix this and correct app cmd (there is depreciation mechanism in moulinette)

@@ -578,6 +579,22 @@ app:
full: --app
help: Write domain & path to app settings for further checks

### app_registerurl()
registerurl:

This comment has been minimized.

Copy link
@zamentur

zamentur Mar 31, 2017

Contributor

Is there a way to mask this kind of command from cli help ?
This command is reserved to package script no ?

yunohost app --help is dreadfull...

This comment has been minimized.

Copy link
@alexAubin

alexAubin Apr 4, 2017

Author Member

We discussed this IRL, but I don't think there's a mechanism at the moment in the Moulinette to 'hide' commands.

@alexAubin
Copy link
Member Author

left a comment

Note : we were discussing with ljf that it might be more meaningful to have this in the "yunohost domain" category instead of "tools" which is too general.

available = False
break

return {"available": available}

This comment has been minimized.

Copy link
@alexAubin

alexAubin Apr 3, 2017

Author Member

We were discussing this with someone (don't remember who) and maybe simply returning the boolean should do the trick. A single boolean is a valid json and we already have a few functions returning a single value. This'd make parsing the output for helpers painless.

@alexAubin alexAubin removed the work needed label Apr 4, 2017

@alexAubin

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2017

(Calling again for merging approval)

@zamentur
Copy link
Contributor

left a comment

Tested and reviewed. LGTM
Note:
It's a bit strange to separate domain and path by a space, but I'm ok to merge like this

@alexAubin alexAubin merged commit f646fdf into unstable Apr 16, 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 split-checkurl branch Apr 16, 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.