Updated example volume paths and url options to help prevent user issues

ssddanbrown commented 1 year ago

[x] I have read the contributing guideline and understand that I have made the correct modifications

Description:

When helping BookStack users with issues, where they are using this container, I commonly see a couple of issues:

Users either leave the volume paths as default or follow the default shown format of having both the app and database containers mount their volumes to the same host directory.
Users leave the APP_URL empty, leading to unexpected issues down the line (Proxy host address being used for example).

This PR changes the defaults to essentially force the APP_URL to be changed, while providing (IMO) sensible defaults for volumes so that users don't mount both containers to the same volume, or find their data is now at /path/to/data.

Benefits of this PR and context:

This helps prevents potential issues, primarily with those less familiar with docker/hosting/linux, who will also require the most support when things go wrong.

These issues aren't actively problematic, but I feel like I'm starting to see these cases (of defaults being used) maybe monthly and I worry about the potential long-term issues of mixing the app data with database data, upon the extra confusion/messiness it may cause for the user.

How Has This Been Tested?

I have built the docs using the local jenkins builder container to ensure they are successfully built as expected.

Source / References:

LinuxServer-CI commented 1 year ago

I am a bot, here are the test results for this PR: https://ci-tests.linuxserver.io/lspipepr/bookstack/v23.05.1-pkg-83222181-pr-171/index.html https://ci-tests.linuxserver.io/lspipepr/bookstack/v23.05.1-pkg-83222181-pr-171/shellcheck-result.xml	Tag	Passed
amd64-v23.05.1-pkg-83222181-pr-171	✅
arm32v7-v23.05.1-pkg-83222181-pr-171	✅
arm64v8-v23.05.1-pkg-83222181-pr-171	✅

thespad commented 1 year ago

From a Bookstack perspective, what's it going to do with a <yourbaseurl> APP_URL?

From our perspective we only do anything with it if there's an existing value in the env file that's different, so it wouldn't change anything there on first-run, but subsequent runs with a "fixed" APP_URL would trigger the message to run the update-url process: https://github.com/linuxserver/docker-bookstack/blob/master/root/etc/s6-overlay/s6-rc.d/init-bookstack-config/run#L99-L109

ssddanbrown commented 1 year ago

@thespad BookStack uses APP_URL for all its own URL generation, so any internal links will be generated with this as the base. We've considered it a required option for a while now, but it can technically be not set without error (For historical back-compat), in which case the system attempts to guess the URL based upon the request, which can often be the proxy in docker environments unless headers are properly configured.

My thought process is that by having a clearly incorrect URL by default, if users miss it initially, they'll look to the config for the URL that they're getting redirected to when accessing the system, where they'll find that URL as part of the option they need to change. Right now users may not connect up the option to the issue they'd face.

In regards to showing the update-url process, I don't think that's a concern from my point of view. This scenario will mainly be during setup where there's little existing data in BookStack so running the command won't really have an affect if the user does so.

I understand the changes here are quite opinionated, based upon my supporting of users. If you think these changes are likely to cause more issues then I totally respect your view and right to close this without merge.

thespad commented 1 year ago

Our only real concern is where providing a default value for an env that the user doesn't bother to change breaks the app (more than leaving it blank). If there's no effective difference between leaving it blank and setting it to an invalid value (other than possibly helping with troubleshooting) then I've no problem with setting it.

That said, if we're going to do this we should probably remove https://github.com/linuxserver/docker-bookstack/blob/master/root/etc/s6-overlay/s6-rc.d/init-bookstack-config/run#L94-L98C1 as it will be redundant (and may be already anyway).

ssddanbrown commented 1 year ago

If there's no effective difference between leaving it blank and setting it to an invalid value (other than possibly helping with troubleshooting) then I've no problem with setting it.

It does have a difference in the event the user takes no action to change defaults. With it filled with an invalid value, It will essentially redirect them to the configured URL on first navigation. This change is effectively forcing the user to set this value, otherwise have an error that directly relates to the set URL in the env.

If you'd prefer not to change the status quo (since it's changing one kind of error scenario for another I'm hoping would be slightly better) I'd respect that and be happy to reduce the PR to just the volume changes, since they're the more significant element.

thespad commented 1 year ago

I don't have an issue with changing the example APP_URL, though for the volume binds can you please add something to identify them as bookstack - lots of people run multiple services in the same compose project and having just ./db_data or ./app_data makes it ambiguous which it relates to (and has a small chance of clashing with an existing folder).

ssddanbrown commented 1 year ago

@thespad Sure! Have now updated the volume binds to contain bookstack.

LinuxServer-CI commented 1 year ago

I am a bot, here are the test results for this PR: https://ci-tests.linuxserver.io/lspipepr/bookstack/v23.05.2-pkg-03a9e252-pr-171/index.html https://ci-tests.linuxserver.io/lspipepr/bookstack/v23.05.2-pkg-03a9e252-pr-171/shellcheck-result.xml	Tag	Passed
amd64-v23.05.2-pkg-03a9e252-pr-171	✅
arm32v7-v23.05.2-pkg-03a9e252-pr-171	✅
arm64v8-v23.05.2-pkg-03a9e252-pr-171	✅

linuxserver / docker-bookstack