search

georchestra / helm-georchestra

geOrchestra helm chart
3 stars 5 forks source link

add webapp prepare initcontainer for security-proxy #53

Closed jeanmi151 closed 9 months ago

jeanmi151 commented 10 months ago

I did it like that for "fast/easyway" to customize 404.jsp,

Tested and working

jeanpommier commented 10 months ago

What's the purpose of this change ? I don't understand

jeanmi151 commented 10 months ago

What's the purpose of this change ? I don't understand

the purpose is to customize easly the 404.jsp page like here : https://dev.datagrandest.fr/gfds

jeanpommier commented 10 months ago

Sorry, my mind is not in the SP right now. Can you explain a bit more in detail what it is doing and why ? So far, what I understand: the idea is to

Why this split behaviour ? Why not putting it all in the datadir and making just a link (ln) at the right place for instance ?

jeanmi151 commented 10 months ago

Sorry, my mind is not in the SP right now. Can you explain a bit more in detail what it is doing and why ? So far, what I understand: the idea is to

* copy the 404 HTML page from an external jetty container

* get the associated static resources from  the datadir /security-proxy/resources/.

Why this split behaviour ? Why not putting it all in the datadir and making just a link (ln) at the right place for instance ?

we don't want all the webapp in the datadir, just little thing we want to customize we are doing like for GN or GS here : https://github.com/georchestra/helm-georchestra/blob/main/templates/geoserver/geoserver-deployment.yaml#L49

  1. copy the webapp ressources in the volume
  2. overide few file we need to overide
  3. mount this volume in the coutainer for it to run with custom webapp
jeanpommier commented 10 months ago

Mmmh, I didn't see that. Why not apply those changes in the docker images's entrypoint, directly ? It might sound a bit picky, but, this is adding some not-very-generic logic into the helm chart, to answer quite specific needs. I'd say either we apply this directly to the docker container because it makes sense, generally, for most usage, or it is very specific for a use-case or a customer and I'm not sure this is the bet way or place to put it

jeanmi151 commented 10 months ago

Why doing it like this ?

  1. Is it done like that for GeoNetwork, Geoserver and CAS
  2. Don't want to go modify repo geOrchestra because process is long
  3. Difficult to remake image with already version tag
  4. If modify geOrchestra the good process would be to create new patch release
jeanpommier commented 10 months ago

I understand. I'm not against it per-se for a quick patch. But my fear is that it is going just to stack patch upon patch and clutter the helm chart with very specific stuff that shouldn't be there is the first place. If we have to do it here, I'd really prefer adding the quite common logic of allowing to define in the values

which, I believe, would allow to do the same, but with more flexibility and genericity. And since this would be cluttering the values file, it's more visible and we might be more prone to port relevant improvements to upstream georchestra code

jeanmi151 commented 10 months ago

@jeanpommier thanks for your comments, I think why want the same :) In the mean time, Do you agree we merge this PR (I will test it is working) and we work on your propositions later with more time ? this ideas would be a great also : https://github.com/georchestra/georchestra/issues/4030 (we discussed about this with @edevosc2c )

jeanpommier commented 10 months ago

Yes, I'm OK with the temporary patch. I believe that with https://github.com/georchestra/georchestra/issues/4030 we are going in the right direction

fvanderbiest commented 9 months ago

I'm sorry, I'm really opposed to "temporary patches". Often found in productions 10 years later... Let's build something robust from the start :-)

EDIT: did not realize the security-proxy probably lives its latest release. As a result, I'm now 0- on this change.

edevosc2c commented 9 months ago

Often found in productions 10 years later...

That's not true for this one, we are going to propose a real solution over this temporary patch: https://github.com/georchestra/georchestra/issues/4030.

The temporary patch will be well gone once #4030 is implemented.