chore: allow you to use the options object to override *all* the new resource limits #7938
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Manifest Files |
There was a problem hiding this comment.
LGTM, just some minor details. I also wonder... should this be a configuration inside Unleash Admin UI rather than an option in config? The main reason I'm asking is because something at startup requires customers to re-deploy/restart Unleash, while a config does not. Maybe this is just a future improvement we can think about if someone is lazy to restart Unleash
src/lib/create-config.ts
Outdated
| environments: Math.max( | ||
| 0, | ||
| parseEnvVarNumber( | ||
| process.env.UNLEASH_ENVIRONMENTS_LIMIT, | ||
| options?.resourceLimits?.environments || 50, | ||
| ), |
There was a problem hiding this comment.
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
| 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, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 👍
|
Regarding
Yes, probably down the road. I think that along with a resource limit dashboard, that'd be very helpful. But it's a future improvement. |
Because you must always have at least one env for unleash to work
thomasheartman
deleted the
chore/1-2730-allow-overriding-all-new-resource-limit-properties-in-oss
branch
This allows us to use different limits for enterprise self-hosted and hosted