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.

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(dsl): Forbid empty environment variables #17035

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
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
21 changes: 10 additions & 11 deletions infra/src/cli/generate-chart-values.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,25 +24,24 @@ const headerComment = `#########################################################

const NON_EMPTYABLE_PROPERTIES = new Set(['SERVERSIDE_FEATURES_ON'])

// Recursive function to filter out empty string properties
const removeEmptyStringProperties = (obj: any): any => {
// Recursive function to filter out (error on) empty string properties
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
Comment on lines +28 to +40
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance error handling and type safety

The function implementation has several areas for improvement:

  1. The error message should include the property name(s) that were empty
  2. The Set comparison logic could be simplified
  3. 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.

}

const writeYamlFile = (filePath: string, content: unknown) => {
const filteredContent = removeEmptyStringProperties(content)
const filteredContent = verifiablyNonEmptyProperties(content)
const doc = new yaml.Document()
doc.contents = doc.createNode(filteredContent, { keepUndefined: false })

Expand Down
Loading