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

thohng · 2022-09-23T08:20:25Z

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

github-actions

Thanks for opening this pull request! Be sure to follow the pull request template!

LinuxServer-CI · 2022-09-23T08:29:29Z

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

thohng · 2022-09-23T08:31:50Z

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

drizuid · 2022-10-04T16:11:50Z

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.

thohng · 2022-10-08T16:37:49Z

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]);
}

drizuid · 2022-10-08T16:57:19Z

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 :)

LinuxServer-CI · 2022-10-08T19:00:56Z

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

thohng · 2022-10-08T19:02:48Z

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

LinuxServer-CI · 2022-10-10T06:41:20Z

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

LinuxServer-CI · 2022-10-10T07:06:21Z

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

LinuxServer-CI · 2022-10-10T08:01:13Z

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

README.md

This reverts commit f033396.

LinuxServer-CI · 2022-10-10T15:11:45Z

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

# Conflicts: # root/etc/cont-init.d/50-config

LinuxServer-CI · 2022-10-11T08:37:20Z

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

drizuid · 2022-10-11T11:55:37Z

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.

nemchik

Please review NetLah-external#1

thohng · 2022-10-11T19:01:30Z

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~~

LinuxServer-CI · 2022-10-11T20:03:10Z

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

LinuxServer-CI · 2022-10-11T20:05:51Z

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

jaytomten · 2022-10-24T14:02:34Z

Awesome. Waiting on this update.

LinuxServer-CI · 2022-11-14T02:15:21Z

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

github-actions · 2022-12-14T02:38:38Z

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.

thohng · 2022-12-15T12:16:02Z

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!

LinuxServer-CI · 2022-12-15T12:26:07Z

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

nemchik · 2022-12-21T22:34:37Z

@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.

# Conflicts: # root/etc/cont-init.d/50-config

LinuxServer-CI · 2022-12-22T03:28:23Z

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

thohng · 2022-12-22T03:37:20Z

@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 NetLah-external#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.

root/etc/cont-init.d/50-config

nemchik · 2022-12-22T13:01:58Z

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

Avoid grep -E, change to regex Co-authored-by: Eric Nemchik <eric@nemchik.com>

LinuxServer-CI · 2022-12-22T14:23:36Z

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

nemchik

LGTM

root/etc/cont-init.d/50-config

LinuxServer-CI · 2022-12-22T16:37:08Z

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

thohng · 2022-12-23T03:36:15Z

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

Extract host and port from DB_HOST

9cf173b

github-actions bot reviewed Sep 23, 2022

View reviewed changes

Support DB_PORT

71750e1

thohng changed the title ~~Extract host and port from DB_HOST to test connection instead of only use default port 3306~~ Extract host and port from DB_HOST also support DB_PORT to test connection instead of only use default port 3306 Oct 8, 2022

Improve grep and seq matching

5fa1132

Update README support DB_PORT

f033396

Add DB_HOST= to .env if previous version not have

10f9699

drizuid requested a review from a team October 10, 2022 12:17

drizuid reviewed Oct 10, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

thohng added 2 commits October 10, 2022 22:59

Revert "Update README support DB_PORT"

f9bb8bc

This reverts commit f033396.

Update README support DB_PORT

60ac384

thohng added 2 commits October 11, 2022 16:19

Merge branch 'master' into improve-nc-mysql-host-port

ab75716

# Conflicts: # root/etc/cont-init.d/50-config

fix readme-vars: properly escaped password

014fb2a

drizuid requested a review from a team October 11, 2022 11:53

nemchik mentioned this pull request Oct 11, 2022

Rework logic to account for ipv6 NetLah-external/docker-bookstack#1

Merged

nemchik requested changes Oct 11, 2022

View reviewed changes

nemchik mentioned this pull request Oct 17, 2022

Cleanup db check so it doesn't spam the console #142

Merged

1 task

github-actions bot added the no-pr-activity label Dec 14, 2022

Merge branch 'linuxserver:master' into improve-nc-mysql-host-port

a8e02b6

github-actions bot removed the no-pr-activity label Dec 16, 2022

Merge branch 'master' into improve-nc-mysql-host-port

8cb08da

# Conflicts: # root/etc/cont-init.d/50-config

thohng requested a review from nemchik December 22, 2022 03:37

nemchik requested changes Dec 22, 2022

View reviewed changes

root/etc/cont-init.d/50-config Outdated Show resolved Hide resolved

Update root/etc/cont-init.d/50-config

987b111

Avoid grep -E, change to regex Co-authored-by: Eric Nemchik <eric@nemchik.com>

thohng requested a review from nemchik December 22, 2022 14:27

nemchik approved these changes Dec 22, 2022

View reviewed changes

aptalca reviewed Dec 22, 2022

View reviewed changes

root/etc/cont-init.d/50-config Outdated Show resolved Hide resolved

sed and grep to support .env.sample upstream comments with #

e2bd737

thohng requested a review from aptalca December 22, 2022 16:34

aptalca approved these changes Dec 22, 2022

View reviewed changes

nemchik merged commit c075b28 into linuxserver:master Dec 22, 2022

thohng deleted the improve-nc-mysql-host-port branch December 23, 2022 03:37

Uh oh!

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

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

Uh oh!

Conversation

thohng commented Sep 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Benefits of this PR and context:

How Has This Been Tested?

Source / References:

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

LinuxServer-CI commented Sep 23, 2022

Uh oh!

thohng commented Sep 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

drizuid commented Oct 4, 2022

Uh oh!

thohng commented Oct 8, 2022

Uh oh!

drizuid commented Oct 8, 2022

Uh oh!

LinuxServer-CI commented Oct 8, 2022

Uh oh!

thohng commented Oct 8, 2022

Uh oh!

LinuxServer-CI commented Oct 10, 2022

Uh oh!

LinuxServer-CI commented Oct 10, 2022

Uh oh!

LinuxServer-CI commented Oct 10, 2022

Uh oh!

Uh oh!

LinuxServer-CI commented Oct 10, 2022

Uh oh!

LinuxServer-CI commented Oct 11, 2022

Uh oh!

drizuid commented Oct 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nemchik left a comment

Choose a reason for hiding this comment

Uh oh!

thohng commented Oct 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LinuxServer-CI commented Oct 11, 2022

Uh oh!

LinuxServer-CI commented Oct 11, 2022

Uh oh!

jaytomten commented Oct 24, 2022

Uh oh!

LinuxServer-CI commented Nov 14, 2022

Uh oh!

github-actions bot commented Dec 14, 2022

Uh oh!

thohng commented Dec 15, 2022

Uh oh!

LinuxServer-CI commented Dec 15, 2022

Uh oh!

nemchik commented Dec 21, 2022

Uh oh!

LinuxServer-CI commented Dec 22, 2022

Uh oh!

thohng commented Dec 22, 2022

Uh oh!

Uh oh!

nemchik commented Dec 22, 2022

Uh oh!

LinuxServer-CI commented Dec 22, 2022

Uh oh!

nemchik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LinuxServer-CI commented Dec 22, 2022

thohng commented Sep 23, 2022 •

edited

Loading

thohng commented Sep 23, 2022 •

edited

Loading

drizuid commented Oct 11, 2022 •

edited

Loading

thohng commented Oct 11, 2022 •

edited

Loading