build(chart): remove duplicates in client/server charts

[ciyer]

Remove duplicate values from chart

/deploy renku-gateway=master #persist

[RenkuBot]

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

[lorenzo-cavazzi]

I think we might want to merge the URLs in the values so that we don't have something like server.serverData.url + server.serverData.prefix anymore, but just uiServerUrl or even better server.url. The same applies to similar values like gatewayUrl that currently comes from different sources: gatewayUrl for the client and server.gateway.url for the server.

In the PR, we define ui.uiserverUrl in the helpers taking it from ui.server.*, which is confusing and not coherent with what we wrote here https://github.com/SwissDataScienceCenter/renku/blob/master/helm-chart/values.yaml.changelog.md#upgrading-to-renku-0150

I suggest aiming for the following structure:

ui:
  gatewayUrl:
  client:
    url:
    *
  server:
    url:
    *
  global:
    *

I would remove all the unnecessary global.* values, an probably also ui.ingress (is it actually used? If yes, then ui.client.ingress and its server counterpart are probably redundant). And give it a look to other values not used anymore, so that we have breaking changes only once and then we resume working with a clean values structure.

P.S: switching to ui.gatewayUrl and to ui.server.url will likely require changes to the server and/or client code. I don't mind if we keep them separate as server.url.base + server.url.prefix (easier to adapt the server code) or we just merge to server.url (easier to adapt for the client code) as long as we define them only once in the values.

[lorenzo-cavazzi]

Thank you, this goes in the right direction!
I would change the baseUrl to client.url to follow the same structure of the gateway.
I'm also tempted to suggest dropping the url+prefix structure for the server. In that case, we would have just a full URL as server.url. The reason is that I find the naming a bit confusing since it's different than what we do with gateway and client; also, url is in fact what is usually defined as origin, and prefix is the pathname.
We might actually just limit to adding a comment to clarify that in the values, or maybe change the actual names so that they are server.origin and server.pathname (+ server.port).

Do you have any preference for how to handle that? I would probably go with the last suggestion (i.e. server.pathname + server.origin + server.port) or the first one (server.url, intended as full URL, and server.port -- that will require a little change to the config file to separate pathanme and origin).

[lorenzo-cavazzi]

I would change also the baseUrl to be client.url

[lorenzo-cavazzi]

Should we add a comment to explain that this URL should be the "origin" (i.e. schema + domain) only?

[lorenzo-cavazzi]

Should we add a comment to explain this is the "pathname"?

[ciyer]

As discussed, this can just be dropped from the values file. We do not need to give admins control over the pathname.

[ciyer]

Thank you, this goes in the right direction! I would change the baseUrl to client.url to follow the same structure of the gateway. I'm also tempted to suggest dropping the url+prefix structure for the server. In that case, we would have just a full URL as server.url. The reason is that I find the naming a bit confusing since it's different than what we do with gateway and client; also, url is in fact what is usually defined as origin, and prefix is the pathname. We might actually just limit to adding a comment to clarify that in the values, or maybe change the actual names so that they are server.origin and server.pathname (+ server.port).

Do you have any preference for how to handle that? I would probably go with the last suggestion (i.e. server.pathname + server.origin + server.port) or the first one (server.url, intended as full URL, and server.port -- that will require a little change to the config file to separate pathanme and origin).

I like your suggestions. I myself stumbled on the fact that the url was not actually the url, which is annoying. I think it should be possible to further clean up the terminology so that the structures have enough flexibility but with sensible names.

[lorenzo-cavazzi]

Thank you! This looks cleaner to me. Just a minor change on the hard-coded "/ui-server" value and it's ready to get merged

[lorenzo-cavazzi]

Keeping the env variable here and using a hard-coded string seems a bit counter-intuitive.
I would completely remove SERVER_PREFIX from here, and just hardcode it in server/src/config.ts in SERVER.prefix.

[lorenzo-cavazzi]

Looks good now, thanks!

[olevski]

Sekhar, as we discussed I took a quick look at this helm chart PR. I think these changes look really good.

Just two minor things.

[olevski]

Thank you!

[andre-code]

🚀

[RenkuBot]

Tearing down the temporary RenkuLab deplyoment for this PR.