Repository navigation
feat: add intOrString type alias for map[string]intOrString annotations - #25
Conversation
Add intOrString as a primitive type alias that resolves to
intstr.IntOrString (k8s.io/apimachinery/pkg/util/intstr). This allows
map[string]intOrString annotations in values.yaml, which generate a
JSON Schema with anyOf: [{type: integer}, {type: string}] for the
additionalProperties. This is needed for PostgreSQL server parameters
that users may provide as either quoted strings ("100") or bare
integers (100).
Signed-off-by: ohotnikov.ivan <[email protected]>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds support for the ChangesintOrString Type Alias Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…schema
Switch the postgresql.parameters field annotation from
{map[string]string} to {map[string]intOrString}, which generates a
JSON Schema with anyOf: [{type: integer}, {type: string}] plus the
x-kubernetes-int-or-string extension. This lets users supply PostgreSQL
parameters as bare integers (max_connections: 100) or quoted strings
(max_connections: "100") without triggering schema validation errors.
The Helm template already coerces any value to a string via
{{ $value | toString | quote }}, so runtime behavior is unchanged.
Bump CI to cozyvalues-gen v1.3.1, which adds intOrString alias support
(see cozystack/cozyvalues-gen#25). The generated Go type changes from
map[string]string to map[string]intstr.IntOrString; the Application spec
is stored as raw JSON so there is no runtime unmarshaling impact.
Signed-off-by: IvanHunters <[email protected]>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for the intOrString alias, mapping it to Kubernetes' intstr.IntOrString type. Changes include updating primitive checks and resolution logic in both the OpenAPI and README generation modules, along with adding relevant unit tests. Feedback suggests extending the implementation to handle default value emission in emitField for Go code generation and providing specific default values in defaultValueForType for README documentation.
| aliasResources = "resources" | ||
| aliasRequest = "request" | ||
| aliasLimit = "limit" | ||
| aliasIntOrString = "intOrString" |
There was a problem hiding this comment.
The intOrString alias should also be handled in the emitField function (around line 812) to ensure that empty string defaults are correctly emitted as +kubebuilder:default:="" in the generated Go code. Currently, it only handles string and quantity explicitly.
| aliasTime = "time" | ||
| aliasObject = "object" | ||
| aliasEmptyObject = "emptyobject" | ||
| aliasIntOrString = "intOrString" |
There was a problem hiding this comment.
The intOrString alias should be added to the defaultValueForType function (around line 1097) to ensure it provides a sensible default value (such as "" or 0) in the generated README table. Without this, it will fall back to the default {} which is incorrect for a type that resolves to a scalar integer or string.
![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
🧹 Nitpick comments (1)
internal/openapi/openapi_test.go (1)
432-434: ⚡ Quick winAssert
anyOf/x-kubernetes-int-or-stringat the exact schema path.Current string-contains checks can pass on unrelated schema locations; assert on
properties.postgresql.properties.parameters.additionalPropertiesinstead.Suggested test hardening
schemaBytes, err := os.ReadFile(outfile) require.NoError(t, err) - schema := string(schemaBytes) - require.Contains(t, schema, `"x-kubernetes-int-or-string": true`) - require.Contains(t, schema, `"anyOf"`) + + var schema map[string]any + require.NoError(t, json.Unmarshal(schemaBytes, &schema)) + + props := schema["properties"].(map[string]any) + pg := props["postgresql"].(map[string]any) + pgProps := pg["properties"].(map[string]any) + parameters := pgProps["parameters"].(map[string]any) + additional := parameters["additionalProperties"].(map[string]any) + + require.Equal(t, true, additional["x-kubernetes-int-or-string"]) + anyOf, ok := additional["anyOf"].([]any) + require.True(t, ok) + require.NotEmpty(t, anyOf) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/openapi/openapi_test.go` around lines 432 - 434, The current string-contains checks are too coarse; instead unmarshal schemaBytes (or use a JSON-path helper) and navigate to the exact node at properties.postgresql.properties.parameters.additionalProperties, then assert that that node includes the keys/values `"x-kubernetes-int-or-string": true` and an `"anyOf"` entry. Concretely: replace the two require.Contains(t, schema, ...) calls with code that json.Unmarshal(schemaBytes, &doc) (or use gjson.GetBytes) then locate doc["properties"]["postgresql"]["properties"]["parameters"]["additionalProperties"] and use require assertions on that map to confirm it has the x-kubernetes-int-or-string boolean and an anyOf array. Ensure you reference the existing variables schemaBytes/schema in the test and keep assertions specific to that node.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/readme/readme.go`:
- Around line 873-874: The function defaultValueForType currently falls through
to an object default for types even though intOrString is marked primitive
alongside aliasQuantity, aliasDuration, aliasTime, aliasObject,
aliasEmptyObject, aliasIntOrString; add a scalar-default branch for the
intOrString case inside defaultValueForType so it returns an appropriate scalar
default (e.g., empty string or zero depending on desired README semantics)
instead of `{}`; locate the switch/if that handles
aliasQuantity/aliasDuration/.../aliasIntOrString and insert the
intOrString-specific return path there.
---
Nitpick comments:
In `@internal/openapi/openapi_test.go`:
- Around line 432-434: The current string-contains checks are too coarse;
instead unmarshal schemaBytes (or use a JSON-path helper) and navigate to the
exact node at properties.postgresql.properties.parameters.additionalProperties,
then assert that that node includes the keys/values
`"x-kubernetes-int-or-string": true` and an `"anyOf"` entry. Concretely: replace
the two require.Contains(t, schema, ...) calls with code that
json.Unmarshal(schemaBytes, &doc) (or use gjson.GetBytes) then locate
doc["properties"]["postgresql"]["properties"]["parameters"]["additionalProperties"]
and use require assertions on that map to confirm it has the
x-kubernetes-int-or-string boolean and an anyOf array. Ensure you reference the
existing variables schemaBytes/schema in the test and keep assertions specific
to that node.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 94e06504-0a9a-4038-a0af-cc5095e8a276
📒 Files selected for processing (3)
internal/openapi/openapi.gointernal/openapi/openapi_test.gointernal/readme/readme.go
|
Closing: the upstream cozystack PR was simplified to a direct schema edit without requiring generator changes. |
Address CodeRabbit review on #25: replace coarse string-contains assertions with json.Unmarshal navigation to properties.postgresql.properties.parameters.additionalProperties, then assert x-kubernetes-int-or-string: true and that anyOf contains exactly {type:integer} and {type:string}. Signed-off-by: ohotnikov.ivan <[email protected]>
|
Reopening — the simplification path didn't work. The simplified approach (direct schema edit in cozystack/cozystack#2687, no generator change) fails because Generator-level support (this PR) is the durable fix. Pushed an additional test commit addressing the CodeRabbit nitpick: the assertion now |
|
Looks good overall, the alias is wired in symmetrically to
Unrelated, but spotted while reading: in |
Address review feedback on #25: - defaultValueForType now returns "" for scalar intOrString instead of falling through to the object default {}. Symmetric with string and quantity, both of which render as empty string when no value is set. - README "Supported value types" table gains an intOrString row with PostgreSQL-style examples so the feature is discoverable. - Add unit + end-to-end tests covering scalar intOrString rendering. The unrelated dead-code observation in readme.go:797-803 (both map[...] branches returning the same map[string]object) is left for a separate change since it pre-dates this PR. Signed-off-by: ohotnikov.ivan <[email protected]>
|
Addressed both blockers:
Added unit + end-to-end tests covering scalar rendering. Re: |
…erator
Ivan's original schema edits were correct in intent but lived in the
generated artifacts (values.schema.json and the embedded openAPISchema
in postgres-rd/cozyrds/postgres.yaml). `make generate` reverts them
because values.yaml's `@field {map[string]string}` annotation is the
source of truth -- same regeneration trap that caught #2466 (issue #2597).
cozyvalues-gen v1.4.0 (cozystack/cozyvalues-gen#25, merged 2026-05-21)
adds an `intOrString` type alias for map values that emits
`additionalProperties.anyOf: [integer, string]` plus
`x-kubernetes-int-or-string: true`. Move the change to source by
flipping the annotation, then regenerate:
- packages/apps/postgres/values.yaml: `@field {map[string]intOrString}`
with updated description noting integer values are accepted.
- packages/apps/postgres/values.schema.json: regenerated -- now carries
anyOf + x-kubernetes-int-or-string: true.
- packages/system/postgres-rd/cozyrds/postgres.yaml: embedded
openAPISchema regenerated to match.
- packages/apps/postgres/README.md: regenerated type column.
- api/apps/v1alpha1/postgresql/types.go: Parameters is now
`map[string]intstr.IntOrString`.
The result survives future regenerations. The template's
`{{ \$value | toString | quote }}` already coerces both inputs to
quoted strings, verified end-to-end with helm template against both
`max_connections: 100` (int) and `max_connections: "100"` (string) --
identical render output. Dangerous-param block list still rejects
archive_cleanup_command and recovery_end_command additions from the
prior commit.
Co-Authored-By: IvanHunters <[email protected]>
Assisted-By: Claude <[email protected]>
Signed-off-by: Myasnikov Daniil <[email protected]>
…erator
Ivan's original schema edits were correct in intent but lived in the
generated artifacts (values.schema.json and the embedded openAPISchema
in postgres-rd/cozyrds/postgres.yaml). `make generate` reverts them
because values.yaml's `@field {map[string]string}` annotation is the
source of truth -- same regeneration trap that caught #2466 (issue #2597).
cozyvalues-gen v1.4.0 (cozystack/cozyvalues-gen#25, merged 2026-05-21)
adds an `intOrString` type alias for map values that emits
`additionalProperties.anyOf: [integer, string]` plus
`x-kubernetes-int-or-string: true`. Move the change to source by
flipping the annotation, then regenerate:
- packages/apps/postgres/values.yaml: `@field {map[string]intOrString}`
with updated description noting integer values are accepted.
- packages/apps/postgres/values.schema.json: regenerated -- now carries
anyOf + x-kubernetes-int-or-string: true.
- packages/system/postgres-rd/cozyrds/postgres.yaml: embedded
openAPISchema regenerated to match.
- packages/apps/postgres/README.md: regenerated type column.
- api/apps/v1alpha1/postgresql/types.go: Parameters is now
`map[string]intstr.IntOrString`.
The result survives future regenerations. The template's
`{{ \$value | toString | quote }}` already coerces both inputs to
quoted strings, verified end-to-end with helm template against both
`max_connections: 100` (int) and `max_connections: "100"` (string) --
identical render output. Dangerous-param block list still rejects
archive_cleanup_command and recovery_end_command additions from the
prior commit.
Co-Authored-By: IvanHunters <[email protected]>
Assisted-By: Claude <[email protected]>
Signed-off-by: Myasnikov Daniil <[email protected]>
…ema (#2687) ## What this PR does PostgreSQL server parameters like `max_connections` are natural integers, but the chart's schema declared them as `{type: string}` only. When users supply bare integers (e.g. `max_connections: 100`) — either directly in Application CR specs or via the UI — Helm rejects the values at schema validation time before template rendering, causing HelmRelease failures. The Helm template already handles both types via `{{ $value | toString | quote }}`, so the fix is to teach the schema to accept both. ### Approach The schema files (`values.schema.json` and the embedded `openAPISchema` in `postgres-rd/cozyrds/postgres.yaml`) are generated from `values.yaml` annotations via `cozyvalues-gen`. Hand-editing them does not survive `make generate` — same regeneration trap that caught #2466 (issue #2597). `cozyvalues-gen` v1.4.0 (cozystack/cozyvalues-gen#25) adds an `intOrString` type alias for map values that emits `additionalProperties.anyOf: [integer, string]` plus the Kubernetes `x-kubernetes-int-or-string: true` marker. ### Changes - `packages/apps/postgres/values.yaml`: annotation flipped from `@field {map[string]string}` to `@field {map[string]intOrString}`, description updated to note integer values are accepted. - Regenerated artifacts: `values.schema.json`, embedded `openAPISchema` in `packages/system/postgres-rd/cozyrds/postgres.yaml`, `packages/apps/postgres/README.md`, and `api/apps/v1alpha1/postgresql/types.go` (Parameters is now `map[string]intstr.IntOrString`). - `packages/apps/postgres/templates/db.yaml`: `$dangerousParams` list extended with `archive_cleanup_command` and `recovery_end_command` (same RCE surface as `archive_command` / `restore_command`). ### Verification - `helm template` with `max_connections: 200` (int) and `max_connections: "200"` (string) produce identical render output. - Dangerous-param block list rejects `archive_cleanup_command` and `recovery_end_command` with the expected `SECURITY:` error. - `make test` in `packages/apps/postgres/`: 13/13 pass. - `make generate` is idempotent — schema files don't drift on regen. ### Release note ```release-note fix(postgres): accept integer values for postgresql.parameters to prevent HelmRelease failures when parameters are supplied as unquoted integers; also block archive_cleanup_command and recovery_end_command (RCE surface). ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * PostgreSQL parameters now accept integer or string values; integers are automatically coerced to strings. * **Security** * Enhanced parameter filtering blocks additional dangerous settings: archive_cleanup_command and recovery_end_command. * **Documentation** * Updated docs and charts to describe int-or-string parameter support and revised blocked-parameter guidance. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/cozystack/cozystack/pull/2687?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
](https://app.coderabbit.ai/change-stack/cozystack/cozystack/pull/2687?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
Summary
intOrStringas a new primitive type alias in bothopenapiandreadmepackagesintstr.IntOrString(k8s.io/apimachinery/pkg/util/intstr) in generated Go codeanyOf: [{type: integer}, {type: string}]+x-kubernetes-int-or-string: truein JSON SchemaMotivation
Some Helm chart parameters (e.g. PostgreSQL server parameters like
max_connections) are naturally expressed as integers, but users sometimes supply them without quoting. The current{map[string]string}annotation rejects integer values at schema validation time, causingHelmReleasefailures for existing clusters that have unquoted integers in their Application spec.With
{map[string]intOrString}, the schema accepts bothmax_connections: "100"(string) andmax_connections: 100(integer), while the Helm template already handles coercion via{{ $value | toString | quote }}.Test plan
TestResolveSpecialAliasesextended to verifyintOrString→intstr.IntOrStringresolution and importTestIntOrStringSchemaGenerationverifies end-to-end schema generation producesx-kubernetes-int-or-string: trueandanyOfgo test ./...)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests