Support for user provided archive access url #75
Conversation
| @@ -1305,6 +1320,17 @@ def collect_resource_keys(urls_data: list) -> list: | |||
| resource_keys.append(key) | |||
| return resource_keys | |||
|
|
|||
| @staticmethod | |||
| def is_valid_url(url: str) -> bool: | |||
There was a problem hiding this comment.
Maybe you can use here a function from urllib, e.g. urlparse()
There was a problem hiding this comment.
Thanks, I saw some people using urlparse but apparently it's too permissive in some cases. The one I got is what is or was used to validate url in Django (and I removed local and ip addresses + other protocols).
I will investigate a bit more
There was a problem hiding this comment.
There is also https://github.com/python-validators/validators which you may consider to use here.
But if regex covers all cases, we can go with it, I think. However, the current regex seems to be a bit wrong, it allows for empty hostname due to | in the end of line 1328.
This could be covered by some kind of test. @bgruening is there an established practice of adding unit-tests (with e.g. pytest) to the tool's script, or it should be incorporated in planemo tests ?
There was a problem hiding this comment.
unit test will not be run. Planemo test is the only tests that we run automatically. That is, we usually don't want to ship complex code with the wrappers. The usual way is that we depend on packages and more complex scripts should be packaged etc ...
However, we could run unit-tests here on CI for all .py scripts, if that helps.
There was a problem hiding this comment.
There is also https://github.com/python-validators/validators which you may consider to use here. But if regex covers all cases, we can go with it, I think. However, the current regex seems to be a bit wrong, it allows for empty hostname due to
|in the end of line 1328. This could be covered by some kind of test. @bgruening is there an established practice of adding unit-tests (with e.g. pytest) to the tool's script, or it should be incorporated in planemo tests ?
I saw that the validators package seemed to be widely used but it's not built in and you have to install it separately if I'm not mistaken.
Thanks for the find I will correct the regex and then we can see if we keep it or use another solution
|
NOTE: https://github.com/esg-epfl-apc/tools-astro/actions/runs/7707857509/job/21005802183?pr=75 fails because the PR is from a fork. So secrets are not accessible. This is expected and I don't know a way around |
It's important that it is so, for security. For that, @francoismg , you can also create a PR from a branch in this repo, no? |
yes I just did, we will see if it works that way #78 |
Thanks! There is a problem in the tool preview deployment. I will fix it |
|
@francoismg you may probably want to close this PR in favour of #78 |
Add the possibility for a user to enter an archive TAP service access url directly