Draft: env variables for ACTION_HISTORY limits

[vviers]

Context

Partial fix of #1121

As a self-hoster, I want to be able to parameterize ACTION_HISTORY max sizes (both as rows and as disk space)

Proposed solution

Introduce 2 env variables to make those limits parametrizable.

Related issues

Has this been tested?

• 👍 yes, I added tests to the test suite
• 💭 no, because this PR is a draft and still needs work
• 🙅 no, because this is not relevant here
• 🙋 no, because I need help

Screenshots / Screencasts

[fflorent]

You may rather consider using the section util, like this (and also for the line above):

const ACTION_HISTORY_MAX_BYTES = section.flag('actionHistoryMaxBytes').requireInt({
      envVar: 'ACTION_HISTORY_MAX_BYTES',
      defaultValue: 1000 * 1000 * 1000,
    });

This way, you are sure about the type, and you have default value.
Also if the passed string is not a number, it raises an error for you and the server crashes immediately (instead of having erratic behavior).

You may take a look at OIDCConfig for an example.

[vviers]

Ah, I used parseInt instead, does that still work ?

[vviers]

Also, I wouldn't know in which "section" to put it ? Should I create a new one ? I'm not too familiar with this object.

[fflorent]

Ah, I used parseInt instead, does that still work ?

To work, you should check that the result of that function does not return NaN and throw otherwise.

Also it feels more future-proof (in case Grist stores the configuration in a file or in the database).

Also, I wouldn't know in which "section" to put it ? Should I create a new one ?

I would say so.

Also I would control the minimum value (and maybe the maximum). There is no such thing in the current implementation of readInt / requireInt, I would suggest to do that. If you need help on it, or need me to make this addition, please ask.

[fflorent]

Also the env variable names should be prefixed with GRIST_: GRIST_ACTION_HISTORY_MAX_ROWS and GRIST_ACTION_HISTORY_MAX_BYTES.

It is a good practice, in order to avoid name collision with other app that would run in the same shell.

[hexaltation]

It seems that the Typescript to do what you want is

Suggested change

	const ACTION_HISTORY_MAX_ROWS = parseInt(process.env.ACTION_HISTORY_MAX_ROWS, 10) || 1000;
	const ACTION_HISTORY_MAX_ROWS = Number(process.env.ACTION_HISTORY_MAX_ROWS) || 1000;

If what you set in env is NaN, let say "test" it will fallback to default value 1000.

[hexaltation]

but @fflorent suggestion is better.

[hexaltation]

Maybe we want to give access to ACTION_HISTORY_GRACE_FACTOR and ACTION_HISTORY_CHECK_PERIOD in env too ?

[fflorent]

I feel like:

• not that simple to explain
• can be dangerous if the value is not controlled

Also YAGNI, so I would leave it as is and let a user either report the need or make a contribution if they really need to

[hexaltation]

Same thing here

Suggested change

	const ACTION_HISTORY_MAX_BYTES = parseInt(process.env.ACTION_HISTORY_MAX_BYTES, 10) || 1000 * 1000 * 1000; // 1 GB.
	const ACTION_HISTORY_MAX_BYTES = Number(process.env.ACTION_HISTORY_MAX_BYTES) || 1000 * 1000 * 1000; // 1 GB.

[fflorent]

Both should work here, I feel like this should make no difference.

[fflorent]

I feel like:

• not that simple to explain
• can be dangerous if the value is not controlled

Also YAGNI, so I would leave it as is and let a user either report the need or make a contribution if they really need to

[fflorent]

Ah, I used parseInt instead, does that still work ?

To work, you should check that the result of that function does not return NaN and throw otherwise.

Also it feels more future-proof (in case Grist stores the configuration in a file or in the database).

Also, I wouldn't know in which "section" to put it ? Should I create a new one ?

I would say so.

Also I would control the minimum value (and maybe the maximum). There is no such thing in the current implementation of readInt / requireInt, I would suggest to do that. If you need help on it, or need me to make this addition, please ask.

[fflorent]

I resume the work on this PR here: #1262