Feature/registration interval config #50
Conversation
lib/sip-trunk-register.js
Outdated
| @@ -114,7 +117,7 @@ class Regbot { | |||
| else if (res.has('Expires')) { | |||
| expires = parseInt(res.get('Expires')); | |||
| } | |||
| if (isNaN(expires) || expires < 30) expires = DEFAULT_EXPIRES; | |||
| if (isNaN(expires) || expires < MIN_EXPIRES) expires = Math.max(MIN_EXPIRES, DEFAULT_EXPIRES); | |||
There was a problem hiding this comment.
what is the scenario where MIN_EXPIRES will possibly be greater than DEFAULT_EXPIRES?
There was a problem hiding this comment.
if we enable setting DEFAULT_EXPIRES via envvar, well, someone could pass a lower value
There was a problem hiding this comment.
it's dumb, but I figured if a minimum is set, then we should comply with it
There was a problem hiding this comment.
why would we allow a default expires value to be less than the minimum? I don't believe in coding to support a case like that, we should rather reject attempts to set meaningless values such as these
There was a problem hiding this comment.
at what level should those attempts be rejected? do we kill the process when loading the envvars? or do we just specify it in the docs and let the code run?
There was a problem hiding this comment.
jambonz/helm-charts#36 includes these new envvars in the helm charts too
There was a problem hiding this comment.
@davehorton would you rather leave it as it was?
if (isNaN(expires) || expires < MIN_EXPIRES) expires = DEFAULT_EXPIRES;
There was a problem hiding this comment.
I would say if expires < MIN_EXPIRES log a message and set it to MIN_EXPIRES
There was a problem hiding this comment.
@davehorton is this more in line with what you had in mind?
…an it or not parseable
|
please rebase |
Allow the user to control the minimum/default outbound registration expire intervals via two new envvars.
Keep the current defaults of 30s and 3600s if the envvars are not defined.