support postgresql other port number

[lionelbargeot]

Closes #

What has been done to verify that this works as intended?

We have configured an external database server that uses another port (5434)
We tested ODK Central succesfully.

Why is this the best possible solution? Were any other approaches considered?

We added DB_PORT environment variable support in docker-compose.yaml and in scripts that uses this en var.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

users can use a postgresql db server that listen another port than default.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

Not specialy.

Before submitting this PR, please make sure you have:

• [] branched off and targeted the next branch OR only changed documentation/infrastructure (master is stable and used in production)
• [] verified that any code or assets from external sources are properly credited in comments or that everything is internally sourced

[matthew-white]

Thank you, @lionelbargeot! We've talked about wanting to add this port option: #494 (comment). It's nice that that's something that will be easy for users to set.

[matthew-white]

The quotes around ${DB_PORT} shouldn't be needed, since ${DB_PORT} will be a number. Central Backend will pass this port option as-is to Knex.js, and from the Knex.js docs, it looks like Knex.js is expecting a number, not a string. Then again, if it's working for you with these quotes, it probably doesn't really matter either way.

[matthew-white]

Just to add to this, I noticed that ${DB_SSL} works similarly in this file. Since it will be either true or false, it doesn't need quotes.

[lionelbargeot]

Hi Matthew, thanks for your feedback. Is there anything else I can help with ?

[matthew-white]

I seem to remember @lognaturel saying that in lieu of a DB_PORT option, some users today include the port in DB_HOST. Is that right?

If so, I think the approach in this PR would actually end up breaking things for those users. This PR always specifies the port config in config.json.template. If DB_PORT is unset, it defaults to 5432. But that won't work out if a user is also specifying a port in DB_HOST. In that case, Central Backend would end up getting two ports: 5432 in the port config and another port via the host config.

Rather than defaulting to 5432, it'd be preferable to specify the port config in config.json.template only if DB_PORT is set to a nonempty string. I'm not sure we have an easy way to do that right now though.

Alternatively, I see in the Central Backend code that specifying null for port is equivalent to omitting port entirely. I think we can leverage that and change the default here from 5432 to null. I'll go ahead and do that now.

[matthew-white]

I think we can leverage that and change the default here from 5432 to null.

It looks like there's some precedence for this: we do something similar for the ssl config.

[matthew-white]

I see in the Central Backend code that specifying null for port is equivalent to omitting port entirely.

Ah I was looking at how Central Backend connects to Slonik, but I'm not so sure that this is true when Backend connects to Knex. I'm looking at knexConnection() in lib/util/db.js. In that case, I think we would end up specifying a null port to Knex. Knex might very well be fine with that, but I'm not sure. I think I'd prefer to make a change to Central Backend so that it filters out a null port before passing the rest of the config to Knex.

[matthew-white]

I searched the codebase for DB_HOST to see if anything else would need to change. Based on that, I'm wondering whether this line would need to incorporate the new DB_PORT variable. I think so.

central/docker-compose.yml

Line 78 in 7114a06

command: [ "wait-for-it", "${DB_HOST:-postgres14}:5432", "--", "./start-odk.sh" ]

The existence of that line and the hard-coded 5432 in it makes me question whether it really would work to include the port in DB_HOST. If we don't know of anyone specific who's taking that strategy, then maybe we should simply default to 5432 instead of null. We could also make a note of all this in release notes in case anyone is taking that approach.