search

nanawel / our-shopping-list

OSL is a simple shared list web-application based on Node and VueJS. Typical uses include shopping lists of course, and any other small todo-list that needs to be used collaboratively.
GNU Affero General Public License v3.0
82 stars 8 forks source link

Healthcheck broken with custom base url #19

Closed kungfoolfighting closed 1 year ago

kungfoolfighting commented 1 year ago

Hi,

I think there is still a small bug in the implementation of the base url feature. Apparently it also changes the base url in the health check url. (at least that is what I am assuming) This url wasn't and can't be adjusted in the dockerfile. So I guess the healthcheck url needs to still work without the base url. I guess this means that you cannot use that same url for health checks from outside of the container? But that sounds like a niche use case anyways.

I can see this line in the logs: [400] Invalid incoming request URL: GET /healthcheck

Hope this isn't too big of fix. Cheers!

nanawel commented 1 year ago

You're perfectly right. I forgot to adapt the Dockerfile accordingly.

nanawel commented 1 year ago

That should do it: https://github.com/nanawel/our-shopping-list/releases/tag/2.7.2

I only ran a quick test as I don't use this feature myself, so tell me if it's okay on your side.

kungfoolfighting commented 1 year ago

Thank you very much again for the super swift fix!

I just gave it a try and got it to work with a small adjustment: After looking at your change, I noticed that you just paste the config variable for the base_url into the url. This doesn't work when the variable is defined like this for example: "BASE_URL: 'osl/'" as it was for me. I had to adjust it to '/osl/ to make it work. Not really a big issue but the readme.md doc contains a few locations where the base url is listed without a leading slash.

nanawel commented 1 year ago

You're right, that's because when it comes to the server or the client sides in JS, I make sure the variable is properly formatted before using it, in order not to rely on the user to make sure the value perfectly respects the expected format and thus avoid having a double slash (or none at all) somewhere which might or might not work in some cases.

As for the Dockerfile now, I'm pretty limited in the normalizations I can perform, but ~I guess an extra-slash here won't be a problem, at least it will still be better than none!~ Well, I had to change one more thing to make it work so that expressJS handles correctly this extra leading slash transparently.

Hope it's finally okay in all cases now :crossed_fingers:

kungfoolfighting commented 1 year ago

The devil is in the details :-) Thanks again!