fix: do not check defaultUrl against list of server options

[olevski]

So when I was implementing this feature, which prevents you from launching a notebook with more cpu/memory than what is specified in the server options, I forgot that the defaultUrl option should not fall in this category. Some people use this to launch notebooks or dashboards, etc on other urls.

/deploy

[olevski]

Currently deployed at https://tasko.dev.renku.ch/

[lorenzo-cavazzi]

It works fine now! 🎉

I'm not sure the regex is really needed. If we want to keep it, I believe it's not working as intended because strings with invalid chars are accepted (see inline comment).
I guess chars as # are valid, and maybe others as well.
I would personally remove it and allow the user to set any string.

[olevski]

It works fine now! 🎉

I'm not sure the regex is really needed. If we want to keep it, I believe it's not working as intended because strings with invalid chars are accepted (see inline comment).
I guess chars as # are valid, and maybe others as well.
I would personally remove it and allow the user to set any string.

I am not sure what characters to include/exclude. I based my list on the characters in a url that do not have to be converted to something else when they are encoded.

However I feel like we should check for valid characters either way. If someone provides characters that do not form a valid url they may not be able to reach the notebook they just successfully launched. Right?

[lorenzo-cavazzi]

Fine for me. In that case, I believe we should include all the admissible chars for a URL.
I would use this as a reference: https://stackoverflow.com/a/1547940/1303090 -- I didn't verify the answer by reading all the linked RFC document, but it seems reasonable.

[olevski]

@lorenzo-cavazzi how about something like this:

fields.Str(
        required=True,
        validate=lambda x: fields.Url().deserialize(urljoin("https://some.host.com/", x))
)

So here we validate with marshmallow's own url field by appending a proper hostname just for the validation. So we dont have to get into validating a url with regex as long as the renku hostname is a proper hostname - which it is. This way we validate the path exactly in the way it will be used.

[lorenzo-cavazzi]

If that allows all the valid URL chars, it's fine (e.g. something like this /tree#localrouting should be allowed).
Otherwise, I would remove the validation because any wrong chars would still end up in a non-working URL, and having an early validation here doesn't provide a better user experience.

To phrase it differently, if I were trying to add something to Jupyter in my project and I get an error because RenkuLab doesn't allow a potentially valid URL required by my custom frontend, I would be upset.
On the other hand, If I actually entered a wrong URL and I get a legit error, I would still prefer no early validation. My browser would translate the invalid chars to some %xy combination, so that I could easily identify the problem.

E.G. with the current solution from this PR, if I try with /tree#1 (which should be a valid URL) I get the following error:

{
  "messages": {
    "json": {
      "serverOptions": {
        "defaultUrl": [
          "Invalid value."
        ]
      }
    }
  }
}

If I try with /tree^1 I get the same error. Without validation, I would simply end up in a /tree%5E1 URL with a 404 error from Jupyter.

[olevski]

@lorenzo-cavazzi I did not end up deploying the fields.Url suggestion I mentioned yesterday - so you tested with the old regex validation. Anyhow, I will just remove all the validations and accept any string. I am not 100% sure what is allowed by the check done by marshmallow's fields.Url.

[olevski]

So the latest version (without url validation) is deployed on my namespace and it works as you described in your latest comment @lorenzo-cavazzi.

[lorenzo-cavazzi]

It looks good now! 🎉
Before merging the PR, it would be great to run the acceptance test -- although I don't really believe this change can disrupt anything.
It's enough to add the string /deploy anywhere in the first message of the PR.

[RenkuBot]

You can access the deployment of this PR at https://renku-ci-nb-542.dev.renku.ch

[lorenzo-cavazzi]

Thanks! All good now that the acceptance tests passed
✔️ https://github.com/SwissDataScienceCenter/renku-notebooks/runs/1870456167