docker-bookstack Extract host and port from DB_HOST also support DB_PORT to test connection instead of only use default port 3306

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

Description:

Improve Bookstack app startup from 30 seconds delayed and trying if the connection to MySQL DB_HOST use the non default port 3306.

Benefits of this PR and context:

To fix issue #135 It will extract the actual host and port from DB_HOST and provide to the nc process. This will support DB_HOST using the endpoint format or host only (with default port 3306):

DB_HOST=hostOrFQDN
DB_HOST=hostOrFQDN:port

Limitations: not support host IPv6

How Has This Been Tested?

Has been tested by mount the new file to the file in docker container

environment:
  - APP_URL=https://domain
  - DB_HOST=10.109.0.2:25060
  - DB_USER=user
  - DB_PASS=password
  - DB_DATABASE=database_name
volumes:
  - /opt/bookstack/config:/config
  - /opt/bookstack/develop/50-config:/etc/cont-init.d/50-config

Logs after fixed

.....
Running config - db_user set
/var/run/s6/etc/cont-init.d/50-config: line 98: warning: command substitution: ignored null byte in input
/var/run/s6/etc/cont-init.d/50-config: line 98: warning: command substitution: ignored null byte in input
Nothing to migrate.
[cont-init.d] 50-config: exited 0.
....

Source / References:

N/A

Sep 23 '22 08:09 thohng

I am a bot, here are the test results for this PR: https://ci-tests.linuxserver.io/lspipepr/bookstack/v22.09.1-pkg-c3f8e947-pr-136/index.html https://ci-tests.linuxserver.io/lspipepr/bookstack/v22.09.1-pkg-c3f8e947-pr-136/shellcheck-result.xml

Sep 23 '22 08:09 LinuxServer-CI

Properly support specific non default port 3306 in DB_HOST and also improve experience for these issues #31, #86 and #88 without working around.

Sep 23 '22 08:09 thohng

I'm a bit unsure why we didn't have DB_PORT as an envvar like other similar containers, and while I like this change a lot, it's inconsistent with our other containers which would have the user specify db_port in compose. we'll discuss this a bit internally.

Oct 04 '22 16:10 drizuid

If we prefer to use DB_PORT, I can submit the PR to support DB_PORT and also solve the 30s delay. I just checked on BookStack, it support both DB_PORT or DB_HOST with port, and port in DB_HOST will take precedence.

https://github.com/BookStackApp/BookStack/blob/development/app/Config/database.php#L45

// MYSQL
// Split out port from host if set
$mysql_host = env('DB_HOST', 'localhost');
$mysql_host_exploded = explode(':', $mysql_host);
$mysql_port = env('DB_PORT', 3306);
if (count($mysql_host_exploded) > 1) {
    $mysql_host = $mysql_host_exploded[0];
    $mysql_port = intval($mysql_host_exploded[1]);
}

Oct 08 '22 16:10 thohng

that would be fine i think. If you take a look at netbox (we have others but netbox is the only one popping into my mind right now) you can see how we are handling DB_PORT, obviously consistency is our goal. I'll make sure this repo is marked for hacktoberfest too, so you can get some credit :)

Oct 08 '22 16:10 drizuid

I am a bot, here are the test results for this PR: https://ci-tests.linuxserver.io/lspipepr/bookstack/v22.09.1-pkg-dc94345f-pr-136/index.html https://ci-tests.linuxserver.io/lspipepr/bookstack/v22.09.1-pkg-dc94345f-pr-136/shellcheck-result.xml

Oct 08 '22 19:10 LinuxServer-CI

Update PR to support DB_PORT

Insert default line DB_PORT=3306 to .env if missing. The current .env.example does not have line DB_PORT=3306
Use port from DB_HOST if any
If no port on DB_HOST, will use DB_PORT if provided, or DB_PORT will be default 3306

The issue #135 is fixed both case port on DB_HOST or DB_PORT provided

Oct 08 '22 19:10 thohng

I am a bot, here are the test results for this PR: https://ci-tests.linuxserver.io/lspipepr/bookstack/v22.09.1-pkg-dc94345f-pr-136/index.html https://ci-tests.linuxserver.io/lspipepr/bookstack/v22.09.1-pkg-dc94345f-pr-136/shellcheck-result.xml

Oct 10 '22 06:10 LinuxServer-CI

I am a bot, here are the test results for this PR: https://ci-tests.linuxserver.io/lspipepr/bookstack/v22.09.1-pkg-dc94345f-pr-136/index.html https://ci-tests.linuxserver.io/lspipepr/bookstack/v22.09.1-pkg-dc94345f-pr-136/shellcheck-result.xml

Oct 10 '22 07:10 LinuxServer-CI

I am a bot, here are the test results for this PR: https://ci-tests.linuxserver.io/lspipepr/bookstack/v22.09.1-pkg-dc94345f-pr-136/index.html https://ci-tests.linuxserver.io/lspipepr/bookstack/v22.09.1-pkg-dc94345f-pr-136/shellcheck-result.xml

Oct 10 '22 08:10 LinuxServer-CI

I am a bot, here are the test results for this PR: https://ci-tests.linuxserver.io/lspipepr/bookstack/v22.09.1-pkg-dc94345f-pr-136/index.html https://ci-tests.linuxserver.io/lspipepr/bookstack/v22.09.1-pkg-dc94345f-pr-136/shellcheck-result.xml

Oct 10 '22 15:10 LinuxServer-CI

I am a bot, here are the test results for this PR: https://ci-tests.linuxserver.io/lspipepr/bookstack/v22.09.1-pkg-dc94345f-pr-136/index.html https://ci-tests.linuxserver.io/lspipepr/bookstack/v22.09.1-pkg-dc94345f-pr-136/shellcheck-result.xml

Oct 11 '22 08:10 LinuxServer-CI

I've asked the team to review the changes, if this was JUST adding DB_PORT i would've merged it but since it also includes the logic for <address>:<port> I'll need team consensus.

Oct 11 '22 11:10 drizuid

Please review NetLah-external#1

Hi Eric Nemchik, I would fix the Alpine 3.15 not support grep -qP and improve the support to domain:port or [::1]:port or [1:2::7:8]:port ~Please help review #141~

Oct 11 '22 19:10 thohng

I am a bot, here are the test results for this PR: https://ci-tests.linuxserver.io/lspipepr/bookstack/v22.09.1-pkg-dc94345f-pr-136/index.html https://ci-tests.linuxserver.io/lspipepr/bookstack/v22.09.1-pkg-dc94345f-pr-136/shellcheck-result.xml

Oct 11 '22 20:10 LinuxServer-CI

I am a bot, here are the test results for this PR: https://ci-tests.linuxserver.io/lspipepr/bookstack/v22.09.1-pkg-dc94345f-pr-136/index.html https://ci-tests.linuxserver.io/lspipepr/bookstack/v22.09.1-pkg-dc94345f-pr-136/shellcheck-result.xml

Oct 11 '22 20:10 LinuxServer-CI

Awesome. Waiting on this update.

Oct 24 '22 14:10 jaytomten

I am a bot, here are the test results for this PR: https://ci-tests.linuxserver.io/lspipepr/bookstack/v22.10.2-pkg-9a84e426-pr-136/index.html https://ci-tests.linuxserver.io/lspipepr/bookstack/v22.10.2-pkg-9a84e426-pr-136/shellcheck-result.xml

Nov 14 '22 02:11 LinuxServer-CI

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Dec 14 '22 02:12 github-actions[bot]

Hi, can we help preview and merge this PR? This PR is not just improvement to support the DB_PORT but also fix the issue that connection can update from docker environment variable to file /config/www/.env. Thanks!

Dec 15 '22 12:12 thohng

I am a bot, here are the test results for this PR: https://ci-tests.linuxserver.io/lspipepr/bookstack/v22.11-pkg-9a84e426-pr-136/index.html https://ci-tests.linuxserver.io/lspipepr/bookstack/v22.11-pkg-9a84e426-pr-136/shellcheck-result.xml

Dec 15 '22 12:12 LinuxServer-CI

@thohng Please check my review comment above requesting small changes. I believe this is nearly ready. Once those changes are made we will re-review and merge this.

Dec 21 '22 22:12 nemchik

I am a bot, here are the test results for this PR: https://ci-tests.linuxserver.io/lspipepr/bookstack/v22.11.1-pkg-9a84e426-pr-136/index.html https://ci-tests.linuxserver.io/lspipepr/bookstack/v22.11.1-pkg-9a84e426-pr-136/shellcheck-result.xml

Dec 22 '22 03:12 LinuxServer-CI

@thohng Please check my review comment above requesting small changes. I believe this is nearly ready. Once those changes are made we will re-review and merge this.

Hi Eric Nemchik, I actually have merged your improvement https://github.com/NetLah-external/docker-bookstack/pull/1 2 months ago, and the PR is ready, but that time I have not triggered the re-review. Please help preview again.

The recent merge PR #151 actually already in my PR 2 months ago, to fix the issue that .env not update again when db info changed (like changing on host, port, username, or password after first time initialized)

I also resolved the conflicts by the PR 151 recently.

Dec 22 '22 03:12 thohng

Wow, I had a pending review that I did not submit. That's entirely my fault. Sorry!

Dec 22 '22 13:12 nemchik

I am a bot, here are the test results for this PR: https://ci-tests.linuxserver.io/lspipepr/bookstack/v22.11.1-pkg-9a84e426-pr-136/index.html https://ci-tests.linuxserver.io/lspipepr/bookstack/v22.11.1-pkg-9a84e426-pr-136/shellcheck-result.xml

Dec 22 '22 14:12 LinuxServer-CI

I am a bot, here are the test results for this PR: https://ci-tests.linuxserver.io/lspipepr/bookstack/v22.11.1-pkg-9a84e426-pr-136/index.html https://ci-tests.linuxserver.io/lspipepr/bookstack/v22.11.1-pkg-9a84e426-pr-136/shellcheck-result.xml

Dec 22 '22 16:12 LinuxServer-CI

New version working well on my side: https://hub.docker.com/layers/linuxserver/bookstack/22.11.1/images/sha256-356ae10976e45c62c57fb15e1ad257b002c3cd133f8e34fb3703ee3484530e01?context=explore

Dec 23 '22 03:12 thohng

docker-bookstack docker-bookstack copied to clipboard

Extract host and port from DB_HOST also support DB_PORT to test connection instead of only use default port 3306

Description:

Benefits of this PR and context:

How Has This Been Tested?

Source / References:

docker-bookstack
docker-bookstack copied to clipboard