Add Microsoft SQL server adapter #100

JongleurNin · 2017-12-21T06:45:03Z

So, Redmine now has proper support for Microsoft SQL Server, which even runs on Linux and Docker now. I attempted to add support for this, and would appreciate feedback. I'm new to this stuff.

While building the Docker images, I had problems with ha.pool.sks-keyservers.net being slow and unreliable, so I'm also proposing changes to use the MIT server instead, which worked much more reliably for me.

Changed "gpg" keyserver to improve reliability.

JongleurNin · 2017-12-21T07:02:31Z

Hmm, that's embarrassing. I'm happy to make any changes that will allow it to pass, but may need some guidance. It appears to be failing because a download tool like wget or curl is not available to download the Phusion Passenger agent binary.

I'm guessing this is because I added wget to the buildDeps section of the Dockerfile.template file. (I had done this because I wasn't able to install the TinyTDS gem needed to support Rails bindings with Microsoft SQL server without it (that's also why I added the other build dependencies). I'll try removing wget from the build dependencies and see if that helps.

JongleurNin · 2017-12-21T07:16:56Z

Travis doesn't seem to like the ampersand in my revised gpg command. I have it in quotes, which worked great when using Docker for Windows, but maybe that's not the best way of addressing it in a cross-platform way.

tianon · 2017-12-21T20:04:03Z

I'd recommend dropping all GPG-related changes from this PR, especially if you want it accepted. 😉

We haven't been 100% happy with ha.pool.sks-keyservers.net, but overall it's definitely more reliable than using a single server was (we used to use pgp.mit.edu, in fact). Some images have gone so far as to loop over several servers to improve build reliability (https://github.com/nginxinc/docker-nginx/blob/77e314a52b206ec18833df66e898c22658d3a486/stable/alpine/Dockerfile#L71-L81), but we didn't want to copy that pattern everywhere since we weren't entirely happy with how verbose it is. Regardless, it's not related to Microsoft SQL, so should definitely not be in this PR. 👍

tianon · 2017-12-21T20:05:09Z

3.2/Dockerfile

@@ -67,12 +67,15 @@ RUN buildDeps=' \
 		libsqlite3-dev \
 		make \
 		patch \
+		libgmp-dev \
+		build-essential \


Using build-essential is always a red flag for me -- here's a quote from the package description (https://packages.debian.org/stretch/build-essential):

If you do not plan to build Debian packages, you don't need this package.

There are usually more specific alternatives that are more appropriate. 👍

I've just tested locally, and I don't think any of these package additions are necessary -- the installation works fine for me with just adding sqlserver to the list of adapters below! 👍

Understood, I'll yank it, see what fails, and replace it with more specific packages.

tianon · 2017-12-21T20:06:08Z

3.2/docker-entrypoint.sh


 			if [ "$MYSQL_PORT_3306_TCP" ] && [ -z "$REDMINE_DB_MYSQL" ]; then
 				export REDMINE_DB_MYSQL='mysql'
 			elif [ "$POSTGRES_PORT_5432_TCP" ] && [ -z "$REDMINE_DB_POSTGRES" ]; then
 				export REDMINE_DB_POSTGRES='postgres'
+			elif [ "$POSTGRES_PORT_1433_TCP" ] && [ -z "$REDMINE_DB_SQLSERVER" ]; then


POSTGRES_PORT_1433_TCP here should be SQLSERVER_PORT_1433_TCP instead

On second thought, I think we should remove this section -- this section is here for backwards compatibility with --link'd containers, and I don't think we need to support that for SQL Server, so this entire elif should just go away.

Thanks for spotting the copy-pasta there.

tianon · 2017-12-21T20:09:16Z

3.2/docker-entrypoint.sh

+				file_env 'REDMINE_DB_USERNAME' "${POSTGRES_ENV_SQLSERVER_USER:-administrator}"
+				file_env 'REDMINE_DB_PASSWORD' "${POSTGRES_ENV_SQLSERVER_PASSWORD}"
+				file_env 'REDMINE_DB_DATABASE' "${POSTGRES_ENV_SQLSERVER_DB:-${REDMINE_DB_USERNAME:-}}"
+				file_env 'REDMINE_DB_ENCODING' 'utf8'


The example at https://www.redmine.org/projects/redmine/wiki/redmineinstall#SQL-Server for sqlserver doesn't include an encoding, so I imagine this should probably default to '' like MySQL does above, right?

Good catch -- on it.

tianon · 2017-12-21T20:10:51Z

3.2/docker-entrypoint.sh

+				file_env 'REDMINE_DB_PORT' '1433'
+				file_env 'REDMINE_DB_USERNAME' "${POSTGRES_ENV_SQLSERVER_USER:-administrator}"
+				file_env 'REDMINE_DB_PASSWORD' "${POSTGRES_ENV_SQLSERVER_PASSWORD}"
+				file_env 'REDMINE_DB_DATABASE' "${POSTGRES_ENV_SQLSERVER_DB:-${REDMINE_DB_USERNAME:-}}"


Same deal here as above -- these should all simply default to '' since we don't need to support pulling credentials from a linked container automatically.

I'm keen to understand this one better. How is this different from the MySQL or PostgreSQL case? If I go with '', will it still be possible to pass in these values when creating a Docker container based on this image?

FWIW, I myself am planning on using an external physical SQL server database, but I can see users wanting to be have the option of linking with a SQL Server container in one shot without having to mess with the database.yml file manually.

Everyone will pass them is as REDMINE_DB_USERNAME, REDMINE_DB_PASSWORD, etc (or the respective _FILE version), and we won't have a fallback to the SQLSERVER_ENV_* variables that come from the environment variables of a container linked as --link my-sqlserver:sqlserver.

Thanks for explaining. So that entire section is just to establish fallbacks/defaults. Right on, I've made everything in that section '' per @tianon's feedback.

tianon · 2017-12-21T20:16:10Z

3.2/Dockerfile

 	' \
 	&& set -ex \
 	&& apt-get update && apt-get install -y $buildDeps --no-install-recommends \
 	&& rm -rf /var/lib/apt/lists/* \
 	&& bundle install --without development test \
-	&& for adapter in mysql2 postgresql sqlite3; do \
+	&& for adapter in sqlserver mysql2 postgresql sqlite3; do \


I'd prefer to keep these in the same order they are in the entrypoint, so sqlserver should go between postgresql and sqlite3. 👍

No problem.

JongleurNin · 2017-12-22T01:39:32Z

Hmm, still failing, but I can't really tell what the problem is -- all gpg changes are backed out. Sample failures appear on these lines, but the image itself appears to be building correctly in the end.

yosifkit · 2017-12-22T01:45:13Z

3.2/Dockerfile

@@ -26,7 +26,7 @@ RUN set -x \
 	&& wget -O /usr/local/bin/tini "https://github.com/krallin/tini/releases/download/$TINI_VERSION/tini-$(dpkg --print-architecture)" \
 	&& wget -O /usr/local/bin/tini.asc "https://github.com/krallin/tini/releases/download/$TINI_VERSION/tini-$(dpkg --print-architecture).asc" \
 	&& export GNUPGHOME="$(mktemp -d)" \
-	&& gpg --keyserver ha.pool.sks-keyservers.net --recv-keys 6380DC428747F6C393FEACA59A84159D7001A4E5 \
+	gpg --keyserver ha.pool.sks-keyservers.net --recv-keys 6380DC428747F6C393FEACA59A84159D7001A4E5 \


I would say the missing && is causing the build failures. 😉

Derp. Thank you!

yosifkit · 2017-12-22T21:01:18Z

3.2/docker-entrypoint.sh


 			if [ "$MYSQL_PORT_3306_TCP" ] && [ -z "$REDMINE_DB_MYSQL" ]; then
 				export REDMINE_DB_MYSQL='mysql'
 			elif [ "$POSTGRES_PORT_5432_TCP" ] && [ -z "$REDMINE_DB_POSTGRES" ]; then
 				export REDMINE_DB_POSTGRES='postgres'
+			elif [ "$SQLSERVER_PORT_1433_TCP" ] && [ -z "$REDMINE_DB_SQLSERVER" ]; then
+				export REDMINE_DB_SQLSERVER='sqlserver'


Should also drop these two lines, since they are also about a linked container.

Will do. But can you help me to understand how this is bad to include for SQL server, but good to include for PostgreSQL and MySQL?

They are still there for backwards compatibility to use the presence of environment variables to detect a --linked container. Docker docs call links a legacy feature that "may eventually be removed". We should instead point users to an example compose/stack file that sets all of the correct environment variables.

tianon · 2017-12-29T18:57:18Z

Dockerfile.template

@@ -67,12 +67,14 @@ RUN buildDeps=' \
 		libsqlite3-dev \
 		make \
 		patch \
+		libgmp-dev \
+		libc6-dev \


Are you sure these are necessary? I was able to install sqlserver successfully without them in my testing. 😕

(If they are indeed necessary, this list should stay in sorted order, but I'm not entirely convinced they are -- maybe they're pulled in already by one of the above deps?)

You might be correct. I haven't been able to figure out how to test this locally due to a variety of Docker errors I encountered. I did have to install these packages before attempting to add support for SQL server in order to build the TinyTDS gem. I will try removing these and seeing whether the CI build passes.

…build TinyTDS gem.

tianon

LGTM! 👍 (and Travis is happy too 🤘)

JongleurNin · 2018-01-03T00:09:32Z

Hmm, Travis was happy for me in my branch too, and I saw it succeed after your merge, but it looks like there are errors in master for 3.4 only. Hope I didn't cause you guys grief somehow!

tianon · 2018-01-03T00:18:33Z

Naw, just a standard flakey test. 👍 ❤️

- `cassandra`: add `docker-entrypoint.sh` to PATH (docker-library/cassandra#130) - `ghost`: 1.19.1 - `haproxy`: 1.6.14, 1.7.10 - `nextcloud`: redis-3.1.6, APCu-5.1.9 - `php`: better libs-preserving code (docker-library/php#556) - `redmine`: add `sqlserver` database adapter (docker-library/redmine#100) - `rocket.chat`: 0.60.3 - `wordpress`: remove `-dev` dependencies (docker-library/wordpress#267)

JongleurNin · 2018-02-07T09:52:31Z

Thanks for merging this, @tianon! You might not read comments on closed PRs, but is there any chance the documentation at https://github.com/docker-library/docs/tree/master/redmine could also be updated so people know that SQL server is now supported and have some examples of how to use it?

JongleurNin added 2 commits December 20, 2017 21:04

Added Microsoft SQL Server support.

6f5c040

Changed "gpg" keyserver to improve reliability.

Removing problematic trailing whitespace.

2e75b78

Removing wget from build dependencies.

ee00c88

tianon reviewed Dec 21, 2017

View reviewed changes

JongleurNin changed the title ~~Add Microsoft SQL server adapter, use more robust gpg keyserver~~ Add Microsoft SQL server adapter Dec 22, 2017

Applying feedback from tianon, backing out gpg keyserver changes.

43cbc4a

yosifkit reviewed Dec 22, 2017

View reviewed changes

Fixing previous attempted fix at restoring gpg keyserver.

1685538

yosifkit reviewed Dec 22, 2017

View reviewed changes

Removing default environment variables for SQL server (per @yosifkit)

5cf25ce

tianon reviewed Dec 29, 2017

View reviewed changes

Removing libgmp-dev and libc6-dev, since they may not be required to …

32d58ce

…build TinyTDS gem.

tianon approved these changes Jan 2, 2018

View reviewed changes

tianon assigned yosifkit Jan 2, 2018

yosifkit merged commit cb871da into docker-library:master Jan 2, 2018

JongleurNin deleted the add-microsoft-sql-server branch January 2, 2018 23:54

tianon mentioned this pull request Jan 3, 2018

Update docker-library images docker-library/official-images#3864

Merged

wglambert mentioned this pull request Sep 14, 2020

MSSQL support is missing #215

Closed

wglambert mentioned this pull request Sep 14, 2020

Add MSSQL variable to Redmine docs docker-library/docs#1797

Merged

Add Microsoft SQL server adapter #100

Add Microsoft SQL server adapter #100

Uh oh!

Conversation

JongleurNin commented Dec 21, 2017

Uh oh!

JongleurNin commented Dec 21, 2017

Uh oh!

JongleurNin commented Dec 21, 2017

Uh oh!

tianon commented Dec 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JongleurNin commented Dec 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tianon left a comment

Choose a reason for hiding this comment

Uh oh!

JongleurNin commented Jan 3, 2018

Uh oh!

tianon commented Jan 3, 2018

Uh oh!

JongleurNin commented Feb 7, 2018

Uh oh!

Uh oh!