chore(dsl): Forbid empty environment variables #17035
Conversation
WalkthroughThe changes in this pull request involve significant modifications to the function that filters properties in an object. The function Changes
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Datadog ReportAll test runs ✅ 68 Total Test Services: 0 Failed, 62 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (5) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
infra/src/cli/generate-chart-values.ts (2)
27-27: Consider improving documentation and naming for NON_EMPTYABLE_PROPERTIESThe constant's purpose and the rationale for allowing specific properties to be empty should be documented. Consider renaming to something more descriptive like
ALLOWED_EMPTY_ENV_VARS.+// Properties that are explicitly allowed to have empty values. +// SERVERSIDE_FEATURES_ON: Empty value indicates all features are disabled -const NON_EMPTYABLE_PROPERTIES = new Set(['SERVERSIDE_FEATURES_ON']) +const ALLOWED_EMPTY_ENV_VARS = new Set(['SERVERSIDE_FEATURES_ON'])
44-44: Improve error handling in writeYamlFileConsider wrapping the
verifiablyNonEmptyPropertiescall with try-catch to provide more context about which file failed validation.- const filteredContent = verifiablyNonEmptyProperties(content) + try { + const filteredContent = verifiablyNonEmptyProperties(content) + } catch (error) { + throw new Error(`Failed to validate content for ${filePath}: ${error.message}`) + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
infra/src/cli/generate-chart-values.ts(1 hunks)
🔇 Additional comments (1)
infra/src/cli/generate-chart-values.ts (1)
27-44: Verify the impact of forbidding empty environment variables
Let's ensure this change doesn't break existing configurations and identify any potential empty environment variables.
| const verifiablyNonEmptyProperties = (obj: unknown): unknown => { | ||
| if (typeof obj !== 'object' || obj === null) return obj | ||
|
|
||
| if (Array.isArray(obj)) { | ||
| return obj.map(removeEmptyStringProperties) | ||
| return obj.map(verifiablyNonEmptyProperties) | ||
| } | ||
|
|
||
| return Object.fromEntries( | ||
| Object.entries(obj) | ||
| .filter( | ||
| ([key, value]) => value !== '' || NON_EMPTYABLE_PROPERTIES.has(key), | ||
| ) // Filter out empty strings | ||
| .map(([key, value]) => [key, removeEmptyStringProperties(value)]), // Recursively apply to nested objects | ||
| ) | ||
| const stripped = Object.entries(obj) | ||
| .filter(([key, value]) => value !== '' || NON_EMPTYABLE_PROPERTIES.has(key)) // Filter out empty strings | ||
| .map(([key, value]) => [key, verifiablyNonEmptyProperties(value)]) // Recursively apply to nested objects | ||
| if (new Set(Object.keys(obj)) !== new Set(Object.fromEntries(stripped))) | ||
| throw new Error('Empty properties not allowed') | ||
| return stripped |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and type safety
The function implementation has several areas for improvement:
- The error message should include the property name(s) that were empty
- The Set comparison logic could be simplified
- Consider using a more specific return type
-const verifiablyNonEmptyProperties = (obj: unknown): unknown => {
+interface JsonValue {
+ [key: string]: JsonValue | string | number | boolean | null | JsonValue[];
+}
+
+const verifiablyNonEmptyProperties = (obj: unknown): JsonValue => {
if (typeof obj !== 'object' || obj === null) return obj
if (Array.isArray(obj)) {
return obj.map(verifiablyNonEmptyProperties)
}
+ const emptyProps: string[] = []
const stripped = Object.entries(obj)
- .filter(([key, value]) => value !== '' || NON_EMPTYABLE_PROPERTIES.has(key))
+ .filter(([key, value]) => {
+ if (value === '' && !ALLOWED_EMPTY_ENV_VARS.has(key)) {
+ emptyProps.push(key)
+ return false
+ }
+ return true
+ })
.map(([key, value]) => [key, verifiablyNonEmptyProperties(value)])
- if (new Set(Object.keys(obj)) !== new Set(Object.fromEntries(stripped)))
- throw new Error('Empty properties not allowed')
+
+ if (emptyProps.length > 0) {
+ throw new Error(`Empty values not allowed for properties: ${emptyProps.join(', ')}`)
+ }
return stripped
}Committable suggestion skipped: line range outside the PR's diff.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #17035 +/- ##
=======================================
Coverage 35.58% 35.58%
=======================================
Files 6933 6933
Lines 146745 146745
Branches 41714 41714
=======================================
Hits 52226 52226
Misses 94519 94519
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
|
When an application uses
env.required()to get a defined environment config variable, it will break at runtime.We recently started removing empty (
'') environment variables, making themunset. This causes applications like the one above to fail if the value it sets is empty, not missing.To prevent empty required config, and avoid missing (empty?) environment variables, let's make empty environment variables be forbidden.
Summary by CodeRabbit
Bug Fixes
New Features