raise severity of non-empty docroot test

[umlaeute]

previously, if the docroot was non-empty, the entrypoint script would wait for 10 seconds and then continue.

now, it will simply fail.
unless the SKIP_CHECK_EMPTY_DOCROOT envvar is set to some non-empty value.

this is a somewhat "breaking change", as people with half-popuplated docroots (e.g. having an local skin; like me) will now experience failures, unless they manually add the SKIP_CHECK_EMPTY_DOCROOT envvar to their setup.

• Closes: wishlist: make timeout on non-empty workdir configurable #384

[pabzm]

Thank you! I would prefer to have this as a non-breaking change, though. We don't have a proper versioning scheme for these image (yet), and I don't want people to have problems with this change.

Could you change it so the default behaviour stays the same?

[umlaeute]

i thought about that, but...

right now (before this PR), the behaviour is to wait 10 seconds if there is data in the docroot. and then continue.

as per #384, some people ("me") find this suboptimal: a better solution is to just skip the wait, if you know that there will be files.
so add a variable to skip the test.

my original proposal was to just set the timeout to 0 (which needs to be done actively by whoever knows that the directory is dirty). this was rejected, as "not solving the root cause".

afact, the "root cause" is that some people find it dangerous if the docroot is not clean.
the 10 second timeout is just a way to give the users a chance to escape this dangerous situation.

so my thinking was:

• if a dirty docroot is a problem, then a simple 10 seconds timeout does not solve this. aborting is a much more explicit signal and actually prevents any harm to the data (and therefore i believe it is preferable)
• if the dirty docroot is not a problem, then the user should take explicit action to resolve it (either cleaning the docroot; or signal (eg via an envvar) that this is ok)

this is what this PR implements.

obviously it breaks backward compatibility, as the image now fails to start for people that rely on dirty docroots (unless they take explicit action).

but how should we solve the backwards compatibility?
adding a signal (an envvar) that the system should warn/wait/abort seems to put the cart before the horse: if I know that I need to be warned, then I am already warned.

i guess it boils down that for me "solving the root cause" means "getting rid of the (arbitrary) timeout" (as this just combines the worst of both worlds: it penalizes but doesn't protect against data loss.

[pabzm]

I understand your thinking and would agree that in principle this approach would be the best. If we would introduce this as a new feature I'd be happy to merge it.

But there's always people out there depending on current behaviour, and I want to avoid breaking their usage unless necessary or really important – especially since we don't have proper version numbers for our images (which we should improve), which could indicate such a breaking change.

That's why I'm asking you to re-introduce the previous behaviour as default in order to merge this.

(I'd like to introduce a semantic versioning scheme for the images, just didn't come around to do it. Once we have that in place I'd be happy to accept this breaking change, too.)

[umlaeute]

totally understood.

but how should it actually look like?

[pabzm]

but how should it actually look like?

Do you mean the behaviour? That should look like before, so by default it checks the directory and if it's not empty, emits the warning and waits 10 seconds, then continues. If the new environment variable is set, then the whole dir check is not performed.

[umlaeute]

so my original suggestion to just allow the user to set the timeout to 0 should be replaced by a simple "boolean"...

# obviously pseudo-code
sleep $(SKIP_CHECK_EMPTY_DOCROOT:-0)?10:0

instead of

sleep ${ROUNDCUBE_EMPTYWORKDIR_TIMEOUT:-10}

i thought the latter was more powerful, but... ¯\(ツ)/¯

(yes, i know: you say "skip the entire test", not just "skip the wait")

[umlaeute]

so how about this:

with a non-empty docroot:

ALLOW_DIRTY_DOCROOT	result
true	ignore
false	abort
(unset)	prolonged timeout of e.g. 60 seconds; print note on using ALLOW_DIRTY_DOCROOT

(true or false could possibly be just "truthy" values)

this will not break any existing systems (that do not have the ALLOW_DIRTY_DOCROOT envvar defined), but raises the penalty to nudge people into taking long-term action (by setting the envvar).
it also uses positive logic (I always hate it if i have to set something to "true" in order to not have an effect).

[github-actions]

@pabzm, @thomascube
🛎️ This PR has had no activity in two weeks.