fix(filebrowser): configure before running

[evilhamsterman]

Fixes #399

Instead of trying to cram the config into one command we check if the db exists, if not initialize it then configure the various values before starting the app.

[matifali]

Hi @evilhamsterman, Could you take care of the tests that are failing?
Also, @dstoffel, can you review and verify if it works for your use case?

Thanks :)

[dstoffel]

it's working correctly, thank you

[evilhamsterman]

So looking at the failing tests they fail even with the older versions of the script. I have tested all those options myself and they all work. What is the expected scripting environment, the script was using #!/usr/bin/env bash for the hash bang and calling bash to execute the script from curl. But the tests appear to be running in the Alpine container which doesn't have bash.

Also it seems something's wrong with how it checks Stdout. Even if I change the container to something like debian:bookworm which has bash and make the script do nothing but echo "Hello world" it doesn't register any output in the test and says it just received an empty string

28 |     const state = await runTerraformApply(import.meta.dir, {
29 |       agent_id: "foo",
30 |     });
31 |     const output = await executeScriptInContainer(state, "debian:bookworm-slim");
32 |     expect(output.exitCode).toBe(0);
33 |     expect(output.stdout).toEqual([
                               ^
error: expect(received).toEqual(expected)

  [
+   ""
-   "👷 Starting filebrowser in background... ",
-   "",
-   "📂 Serving /root at http://localhost:13339 ",
-   "",
-   "Running 'filebrowser -d filebrowser.db' ",
-   "",
-   "📝 Logs at /tmp/filebrowser.log"
  ]

- Expected  - 7
+ Received  + 1

      at <anonymous> (/home/dmills/development/coder-modules/filebrowser/main.test.ts:33:27)
✗ filebrowser > runs with default [546.26ms]
46 |       agent_id: "foo",
47 |       database_path: ".config/filebrowser.db",
48 |     });
49 |     const output = await executeScriptInContainer(state, "debian:bookworm-slim");
50 |     expect(output.exitCode).toBe(0);
51 |     expect(output.stdout).toEqual([
                               ^
error: expect(received).toEqual(expected)

  [
+   ""
-   "\u001B[0;1mInstalling filebrowser ",
-   "",
-   "🥳 Installation complete! ",
-   "",
-   "👷 Starting filebrowser in background... ",
-   "",
-   "📂 Serving /root at http://localhost:13339 ",
-   "",
-   "Running 'filebrowser --noauth --root /root --port 13339 -d .config/filebrowser.db --baseurl ' ",
-   "",
-   "📝 Logs at /tmp/filebrowser.log"
  ]

- Expected  - 11
+ Received  + 1

[mafredri]

Had some minor suggestions, but didn't spot anything terribly wrong that wasn't already. 😄

Also, since this is a Bash dependent script, we should use [[ instead of [. The former has many benefits and no downsides compared to [. (Not requiring this change though, as it's what was used before.)

[mafredri]

I want to make it clear that this issue was not introduced by this PR.

But I'll just mention this again; this will break if DB_PATH has spaces in it. A better way would be to initialize this as an array and use it in a way that handles spaces:

args=()
if [ "${DB_PATH}" != "filebrowser.db" ]; then
	args+=(-d "${DB_PATH}")
fi

Then use this as:

filebrowser "${args[@]}"

(I did not verify if $$ vs $ is required here due to terraform templates.)

This syntax works even in bash-3.2, so should be fine to use. (I opted for arrays as I see we already have a heavy dependency on Bash here.)

[evilhamsterman]

TBH There's a lot of places that it could break because of spaces in the path, the LOG_PATH and SERVER_BASE_PATH could also cause problems.

I do think I found a bit easier and more readable way to handle it. Filebrowser supports the FB_DATABASE environment variable. So I just set that at the top inside quotes and then it doesn't matter, it works with or without spaces.

[matifali]

Hi @evilhamsterman, thank you for your contribution. Could you please address the review comments to merge this PR?

[evilhamsterman]

@matifali Sorry I've had a very busy month. I should be able to get to it later this week

[evilhamsterman]

I applied some of the suggestions above, I also resolved the issue of spaces in the DB path by using the FB_DATABASE environment variable instead. Let me know if there's anything else

[Copilot]

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

[phorcys420]

@evilhamsterman the reason our tests are weird (even on main) is due to the fact the script is non-strict and ignores any stderr output, so the logs are correct even though everything is broken, basically meaning the tests are useless in this context.

the scripts should exit when errors like this occur, but they don't, i am puzzled as to how it even manages to run the script without bash though.

see this example

[phorcys420]

I have confirmed that this works by creating a template on our internal instance.

[phorcys420]

@matifali let's merge it with the broken tests and i'll fix the tests afterwards, could you force merge?

this PR is not the culprit for the broken tests, but our script is not strict enough, and our test suite will not fail on main even though the dependencies (bash and curl) are missing, I'm not sure why it's failing on this PR but the tests are wrong anyways so let's fix this afterwards.

we should look into having stricter values for our scripts like we do in coder/coder where set -euo pipefail is the standard, in this case we should at least have set -e in my opinion, this would at least make it so that the tests fail when they have to.

cc @mafredri for input

[matifali]

Force merged.

[evilhamsterman]

Thanks for the help!