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: allow you to use the options object to override *all* the new resource limits #7938

Merged
merged 4 commits into from
Aug 21, 2024
Merged

chore: allow you to use the options object to override *all* the new resource limits #7938

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0b2617c
chore: update type
thomasheartman Aug 20, 2024
0539bad
chore: allow overriding all new resource limit properties in OSS
thomasheartman Aug 20, 2024
9c7a0d8
chore: only use fallback if options are undefined
thomasheartman Aug 21, 2024
5930f02
chore: set min envs to 1.
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
31 changes: 23 additions & 8 deletions src/lib/create-config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -657,7 +657,7 @@ export function createConfig(options: IUnleashOptions): IUnleashConfig {
1,
parseEnvVarNumber(
process.env.UNLEASH_FEATURE_ENVIRONMENT_STRATEGIES_LIMIT,
30,
options?.resourceLimits?.featureEnvironmentStrategies || 30,
),
),
constraintValues: Math.max(
Expand All @@ -669,23 +669,38 @@ export function createConfig(options: IUnleashOptions): IUnleashConfig {
),
constraints: Math.max(
0,
parseEnvVarNumber(process.env.UNLEASH_CONSTRAINTS_LIMIT, 30),
parseEnvVarNumber(
process.env.UNLEASH_CONSTRAINTS_LIMIT,
options?.resourceLimits?.constraints || 30,
thomasheartman marked this conversation as resolved.
Show resolved Hide resolved
),
),
environments: parseEnvVarNumber(
process.env.UNLEASH_ENVIRONMENTS_LIMIT,
50,
environments: Math.max(
0,
parseEnvVarNumber(
process.env.UNLEASH_ENVIRONMENTS_LIMIT,
options?.resourceLimits?.environments || 50,
),
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need the max here? If you're trying to protect from negative values, I don't think this is going to work, because we'll end up with zero as the constraint and that's probably not what we want

Suggested change
environments: Math.max(
0,
parseEnvVarNumber(
process.env.UNLEASH_ENVIRONMENTS_LIMIT,
options?.resourceLimits?.environments || 50,
),
environments: parseEnvVarNumber(
process.env.UNLEASH_ENVIRONMENTS_LIMIT,
options?.resourceLimits?.environments ?? 50,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I do think that's what we want. We want to disallow negative values, but we can set it to 0 if you want to.

Copy link
Contributor

Choose a reason for hiding this comment

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

What if we have a function parseEnvVarPositiveNumber or we validate afterwards. Because, I believe it'd be a misconfiguration that should throw a runtime error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we can do that. Been thinking of something like it. I don't think I agree that we should throw an error, though. It feels more like just stopping you from doing something stupid. "Be liberal in what you accept", right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but if you set the value to zero because they're doing something stupid, the users will not be able to add new environments, and they wouldn't know why. Because the person configuring Unleash is probably a different person than the one using it, I'd like Unleash to fail fast and tell me what I'm doing wrong... if someone sets a negative value, it's likely they're trying to accomplish something... something different than setting the value to zero

Copy link
Contributor Author

@thomasheartman thomasheartman Aug 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, we talked about the lower limits with @kwasniew. But I agree, for envs, it should be 1, because you can never have fewer than a single env.

But effectively it'll stop you from creating new ones, yeah. But again, if you do that, you probably have a reason for it. If it's an issue for the users, they'll get warnings in the UI and can talk to the admin.

If we think that Unleash should fail at invalid values (like negative limits), that might be a discussion worth having, but I don't think you and I are gonna solve it in this PR. Especially because we seem to disagree about the right course of action here 💁🏼

Another point, though, I think we can mention the minimum allowed values in the docs, to make it clearer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that's fair. My point is that customer adding a negative value is a mistake, if they're doing that intentionally, there's something they want to do, and we should let them know immediately that what they're trying to do (whatever it is) is not possible. And in the case of they mistakenly adding a negative value, I think we should also tell them upfront (e.g. if they typed -10 instead of 10, now they'll get 1, which is probably not what they intended).

In any case, I think both are edge cases and I'm fine not getting to a conclusion here, and I'm also fine you moved on with the PR 👍

),
projects: Math.max(
1,
parseEnvVarNumber(process.env.UNLEASH_PROJECTS_LIMIT, 500),
parseEnvVarNumber(
process.env.UNLEASH_PROJECTS_LIMIT,
options?.resourceLimits?.projects || 500,
),
),
apiTokens: Math.max(
0,
parseEnvVarNumber(process.env.UNLEASH_API_TOKENS_LIMIT, 2000),
parseEnvVarNumber(
process.env.UNLEASH_API_TOKENS_LIMIT,
options?.resourceLimits?.apiTokens || 2000,
),
),
segments: Math.max(
0,
parseEnvVarNumber(process.env.UNLEASH_SEGMENTS_LIMIT, 300),
parseEnvVarNumber(
process.env.UNLEASH_SEGMENTS_LIMIT,
options?.resourceLimits?.segments || 300,
),
),
featureFlags: Math.max(
1,
Expand Down
12 changes: 11 additions & 1 deletion src/lib/types/option.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,17 @@ export interface IUnleashOptions {
dailyMetricsStorageDays?: number;
rateLimiting?: Partial<IRateLimiting>;
resourceLimits?: Partial<
Pick<ResourceLimitsSchema, 'constraintValues' | 'featureFlags'>
Pick<
ResourceLimitsSchema,
| 'apiTokens'
| 'constraintValues'
| 'constraints'
| 'environments'
| 'featureEnvironmentStrategies'
| 'featureFlags'
| 'projects'
| 'segments'
>
>;
}

Expand Down
Loading