Simplify db keepalive startup setting

[systemcrash]

(i.e. whether a comma follows it or not)

Ensures that the keepalive boolean setting can appear on any row in the block (and optionally have a comma follow it) following later edits.

[lmangani]

Why are we not simply making this a default in the JSON without the sed hackery?

[adubovikov]

I have just changed the code and set the keepalive param to true by default. This settings it's not need anymore. Anyway, thank you for your request. I will close it.

[systemcrash]

It IS a default. Some will want to change it.

Problem is that it’s not quoted. You’re welcome to make your own contraption which will do the same job.

On Tue, 19 May 2020 at 10:06, Lorenzo Mangani notifications@github.com wrote:

Why are we not simply making this a default in the JSON without the sed hackery?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/sipcapture/homer-app/pull/349#issuecomment-630657424, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAE6DUMY5IAUUZ4DLHKSGKLRSI4Y5ANCNFSM4NEAGXEQ .

[systemcrash]

Sure. If jq is available in the image. I think making this the default and not being able to switch it off is a bit much. The mechanism floods the logs with the ping...

On Tue, 19 May 2020 at 10:20, Lorenzo Mangani notifications@github.com wrote:

@lmangani commented on this pull request.

I appreciate this a LOT so dont get me wrong - my impression is we're making this too delicate and vulnerable rather rather than simple - This is a JSON config and it was chosen to keep things simple, so how about we just make this a default in the JSON config the docker builder uses since its mandatory for stability? sed is a last resort to configure containers and should only be used to replace a known default values (this is why it uses a copy of the config not the original) otherwise we can use jq perhaps without the regex matching? Thanks again for looking into this!

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/sipcapture/homer-app/pull/349#pullrequestreview-414209666, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAE6DUN3FAKGLBIS6CN3F5LRSI6LZANCNFSM4NEAGXEQ .

[lmangani]

@systemcrash I think you misunderstood. The docker container uses its own fork of the config where parameters have a unique replacement key. So what i would do is the following as per other seeings:

Make the default a key (not true or false yet)

 "database_config": {
    "user": "homer_user",
    "pass": "homer_password",
    "name": "homer_config",
    "host": "homer_db_host",
    "keepalive": homer_db_keepalive
  },

Have the script deploy the default or setting:

DB_KEEPALIVE=${DB_KEEPALIVE:-true}
 if [ -n "$DB_KEEPALIVE" ]; then sed -i "s/homer_db_keepalive/${DB_KEEPALIVE}/g" /usr/local/homer/etc/webapp_config.json; fi

This way it doesn't matter where the setting is (or are) and there's no JSON complications

[systemcrash]

What did I misunderstand?:

I have just changed the code and set the keepalive param to true by default. This settings it's not need anymore. Anyway, thank you for your request. I will close it.

[systemcrash]

@systemcrash I think you misunderstood. The docker container uses its own fork of the config where parameters have a unique replacement key. So what i would do is the following as per other seeings:

Make the default a key (not true or false yet)

 "database_config": {
    "user": "homer_user",
    "pass": "homer_password",
    "name": "homer_config",
    "host": "homer_db_host",
    "keepalive": homer_db_keepalive
  },

Have the script deploy the default or setting:

DB_KEEPALIVE=${DB_KEEPALIVE:-true}
 if [ -n "$DB_KEEPALIVE" ]; then sed -i "s/homer_db_keepalive/${DB_KEEPALIVE}/g" /usr/local/homer/etc/webapp_config.json; fi

This way it doesn't matter where the setting is (or are) and there's no JSON complications

Much simpler. I was going on the assumption that it should be one or the other already at startup, to avoid other problems. ( plus setting it to homer_db_keepalive triggers JSON parse errors )

[lmangani]

Much simpler. I was going on the assumption that it should be one or the other already at startup, to avoid other problems. ( plus setting it to homer_db_keepalive triggers JSON parse errors )

Nice! btw I tried to find a way to use the real config sample, but ultimately decided to provide the containers with a custom config fork and the above approach - the only challenge is we need to keep it in sync but its worth it in terms of simplicity and predictability.

[systemcrash]

OK - all fixed, with some to_lower magic in there. Should avoid any weird shells or ppl writing TRUE or True (JSON parse error)

[lmangani]

Thanks for your contribution and interaction!

[lmangani]

I'm triggering a rebuild of the containers to test it next. Shall be online in about 10m