Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: add new parsing function for env var numbers #7947

Closed
wants to merge 3 commits into from
Closed

chore: add new parsing function for env var numbers #7947

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
04ae667
chore: add new parse function with tests
thomasheartman Aug 21, 2024
d881de3
chore: use new parsing function
thomasheartman Aug 21, 2024
9767b48
chore: rewrite to use original function instead
thomasheartman Aug 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 51 additions & 50 deletions src/lib/create-config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -653,61 +653,62 @@ export function createConfig(options: IUnleashOptions): IUnleashConfig {
process.env.UNLEASH_SIGNAL_TOKENS_PER_ENDPOINT_LIMIT,
5,
),
featureEnvironmentStrategies: Math.max(
1,
parseEnvVarNumber(
process.env.UNLEASH_FEATURE_ENVIRONMENT_STRATEGIES_LIMIT,
options?.resourceLimits?.featureEnvironmentStrategies ?? 30,
),
),
constraintValues: Math.max(
1,
parseEnvVarNumber(
process.env.UNLEASH_CONSTRAINT_VALUES_LIMIT,
options?.resourceLimits?.constraintValues ?? 250,
),
),
constraints: Math.max(
0,
parseEnvVarNumber(
process.env.UNLEASH_CONSTRAINTS_LIMIT,
options?.resourceLimits?.constraints ?? 30,
),
featureEnvironmentStrategies: parseEnvVarNumber(
process.env.UNLEASH_FEATURE_ENVIRONMENT_STRATEGIES_LIMIT,
30,
{
min: 1,
optionsOverride:
options?.resourceLimits?.featureEnvironmentStrategies,
},
),
environments: Math.max(
1,
parseEnvVarNumber(
process.env.UNLEASH_ENVIRONMENTS_LIMIT,
options?.resourceLimits?.environments ?? 50,
),
constraintValues: parseEnvVarNumber(
process.env.UNLEASH_CONSTRAINT_VALUES_LIMIT,
250,
{
min: 1,
optionsOverride: options?.resourceLimits?.constraintValues,
},
),
projects: Math.max(
1,
parseEnvVarNumber(
process.env.UNLEASH_PROJECTS_LIMIT,
options?.resourceLimits?.projects ?? 500,
),
constraints: parseEnvVarNumber(
process.env.UNLEASH_CONSTRAINTS_LIMIT,
30,
{
min: 0,
optionsOverride: options?.resourceLimits?.constraints,
},
),
apiTokens: Math.max(
0,
parseEnvVarNumber(
process.env.UNLEASH_API_TOKENS_LIMIT,
options?.resourceLimits?.apiTokens ?? 2000,
),
environments: parseEnvVarNumber(
process.env.UNLEASH_ENVIRONMENTS_LIMIT,
50,
{
min: 1,
optionsOverride: options?.resourceLimits?.environments,
},
),
segments: Math.max(
0,
parseEnvVarNumber(
process.env.UNLEASH_SEGMENTS_LIMIT,
options?.resourceLimits?.segments ?? 300,
),
projects: parseEnvVarNumber(process.env.UNLEASH_PROJECTS_LIMIT, 500, {
min: 1,
optionsOverride: options?.resourceLimits?.projects,
}),
apiTokens: parseEnvVarNumber(
process.env.UNLEASH_API_TOKENS_LIMIT,
2_000,
{
min: 0,
optionsOverride: options?.resourceLimits?.apiTokens,
},
),
featureFlags: Math.max(
1,
parseEnvVarNumber(
process.env.UNLEASH_FEATURE_FLAGS_LIMIT,
options?.resourceLimits?.featureFlags ?? 5000,
),
segments: parseEnvVarNumber(process.env.UNLEASH_SEGMENTS_LIMIT, 300, {
min: 0,
optionsOverride: options?.resourceLimits?.segments,
}),
featureFlags: parseEnvVarNumber(
process.env.UNLEASH_FEATURE_FLAGS_LIMIT,
5_000,
{
min: 1,
optionsOverride: options?.resourceLimits?.featureFlags,
},
),
};

Expand Down
22 changes: 22 additions & 0 deletions src/lib/util/parseEnvVar.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,25 @@ test('parseEnvVarStringList', () => {
expect(parseEnvVarStrings('a,b,c', [])).toEqual(['a', 'b', 'c']);
expect(parseEnvVarStrings(' a,,,b, c , ,', [])).toEqual(['a', 'b', 'c']);
});

describe('parseEnvVarNumberWithBounds', () => {
test('prefers options override over default value if present', () => {
expect(parseEnvVarNumber('', 1, { optionsOverride: 2 })).toEqual(2);
});

test('accepts 0 as options override', () => {
expect(parseEnvVarNumber('', 1, { optionsOverride: 0 })).toEqual(0);
});

test('prefers env var over options override', () => {
expect(parseEnvVarNumber('5', 1, { optionsOverride: 2 })).toEqual(5);
});

test('clamps the number to greater than or equal to the min number if provided', () => {
expect(parseEnvVarNumber('1', 0, { min: 2 })).toEqual(2);
expect(parseEnvVarNumber('', 0, { min: 2 })).toEqual(2);
expect(
parseEnvVarNumber('', 0, { optionsOverride: 1, min: 2 }),
).toEqual(2);
});
});
23 changes: 17 additions & 6 deletions src/lib/util/parseEnvVar.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,25 @@
export function parseEnvVarNumber(
envVar: string | undefined,
defaultVal: number,
options?: { min?: number; optionsOverride?: number },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was actually thinking of having another function, but having the min here might be ok. When I see options it looks like the function is doing too much.

Maybe instead of having:

parseEnvVarNumber(
            process.env.UNLEASH_FEATURE_ENVIRONMENT_STRATEGIES_LIMIT,
            30,
            {
                min: 1,
                optionsOverride:
                    options?.resourceLimits?.featureEnvironmentStrategies,
            },
        )

you can do something like:

validateMin(1, parseEnvVarNumber(
            process.env.UNLEASH_FEATURE_ENVIRONMENT_STRATEGIES_LIMIT,
            options?.resourceLimits?.featureEnvironmentStrategies ?? 30
        ))

My idea was that this should fail if you provide the wrong value, leaning on fail fast rather than allowing you to have issues at runtime.

But if we just want to ignore the user input (in this case the negative value), I'd stick with your solution of using Math.max function from before.

I still think that in this case of a startup configuration parameter, it's best to fail fast than to assume the customer input is invalid and then add a minimum value of our own, because we might be doing something the customer does not expect

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I see options it looks like the function is doing too much.

Just for clarity: are you talking about the whole options object, or the optionsOverride property?

I still think that in this case of a startup configuration parameter, it's best to fail fast than to assume the customer input is invalid and then add a minimum value of our own

Yeah, and I still don't agree with this. I don't think we do this with any other values, right? Sure, if we can't connect to the database, we die, but not because we verify it.

Also, what would be the use case for setting negative numbers? Or in this case, 0 for strategies? Without strategies, Unleash doesn't work (or you know, flags don't do anything). (But then again, we all know users do the wildest things, so 💁🏼 )

I'd stick with your solution of using Math.max function from before.

Yeah, I don't mind that, to be honest. The one thing I do think that's nice here is the fact that it does the ?? for us and that it's tested. But it's pretty small, and not that important anyway 🤷🏼

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, we could also just define another function in the same file and use that when defining the values:

    const resourceLimit = ({
        envVarName,
        min,
        fallback,
        option,
    }) =>
        Math.max(
            min,
            parseEnvVarNumber(process.env[envVarName], option ?? fallback),
        );

        featureEnvironmentStrategies: resourceLimit({
            envVarName: 'UNLEASH_FEATURE_ENVIRONMENT_STRATEGIES_LIMIT',
            min: 1,
            fallback: 30,
            option: options?.resourceLimits?.featureEnvironmentStrategies,
        })

But it doesn't really address any of the other bits here and it'd be untested.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for clarity: are you talking about the whole options object, or the optionsOverride property?

About the whole options object that includes the optionsOverride. Usually, having an options parameter is a code-smell because means that the function is less composable and it fulfills more than one purpose (which can be modified or disabled by the use of different options).

Also, what would be the use case for setting negative numbers? Or in this case, 0 for strategies? Without strategies, Unleash doesn't work (or you know, flags don't do anything). (But then again, we all know users do the wildest things, so 💁🏼 )

Indeed, the whole point of failing is to tell them not to add a negative number. Probably their intention is different than just having the value 1 (otherwise they would just configure 1, right?). So, I rather tell them, "hey! this is invalid, you can't do this" instead of: "Yeah, I see you want to set a negative number, but I will assume you wanted to say 1 because I don't understand why you want to set this to negative" (now that I think about it, maybe they want infinite... and one would be way lower than infinite)

): number {
if (!envVar) {
return defaultVal;
}
const parsed = Number.parseInt(envVar, 10);
const parse = (fallback: number) => {
if (!envVar) {
return fallback;
}
const parsed = Number.parseInt(envVar, 10);

if (Number.isNaN(parsed)) {
return fallback;
}

return parsed;
};

const parsed = parse(options?.optionsOverride ?? defaultVal);

if (Number.isNaN(parsed)) {
return defaultVal;
if (options?.min) {
return Math.max(options?.min, parsed);
}

return parsed;
Expand Down