CF-3931 : Reject mis-shaped --statement-defaults instead of silently ignoring it#3397
Open
Paras Negi (paras-negi-flink) wants to merge 2 commits into
Open
CF-3931 : Reject mis-shaped --statement-defaults instead of silently ignoring it#3397Paras Negi (paras-negi-flink) wants to merge 2 commits into
Paras Negi (paras-negi-flink) wants to merge 2 commits into
Conversation
|
🎉 All Contributor License Agreements have been signed. Ready to merge. |
Copilot started reviewing on behalf of
Paras Negi (paras-negi-flink)
July 6, 2026 04:06
View session
There was a problem hiding this comment.
Pull request overview
Tightens parsing for the on-prem confluent flink environment create|update --statement-defaults flag so structurally valid-but-mis-shaped JSON/YAML is rejected with a clear error, and updates help/fixtures to document the expected shape.
Changes:
- Switch defaults parsing to strict JSON/YAML decoding to surface unknown fields instead of silently dropping them.
- Document the expected
--statement-defaultsJSON shape in command help output. - Add/adjust integration tests and golden fixtures to assert failure for wrong-shape
--statement-defaults.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/flink/command_environment_create.go | Implements strict JSON/YAML decoding helpers and uses them for defaults parsing; updates --statement-defaults help text. |
| internal/flink/command_environment_update.go | Updates --statement-defaults help text for the update command. |
| test/flink_onprem_test.go | Adds integration test cases asserting invalid --statement-defaults input fails for create/update. |
| test/fixtures/output/flink/environment/create-statement-defaults-invalid.golden | New golden asserting the parse error for invalid statement defaults (create). |
| test/fixtures/output/flink/environment/update-statement-defaults-invalid.golden | New golden asserting the parse error for invalid statement defaults (update). |
| test/fixtures/output/flink/environment/create-help-onprem.golden | Updates help golden with documented expected shape for --statement-defaults. |
| test/fixtures/output/flink/environment/update-help-onprem.golden | Updates help golden with documented expected shape for --statement-defaults. |
| test/fixtures/output/flink/environment/create-no-namespace.golden | Updates help golden with documented expected shape for --statement-defaults. |
| test/fixtures/output/flink/environment/missing-flag-failure.golden | Updates help golden with documented expected shape for --statement-defaults. |
Comments suppressed due to low confidence (1)
test/flink_onprem_test.go:272
- The new strict decoding also applies to YAML input when the flag value is a path ending in .yml/.yaml, but the added integration tests only cover the inline JSON case. Adding a YAML fixture file with an intentionally mis-shaped statement defaults document (and an assertion it fails) would help ensure yaml.Decoder.KnownFields(true) is exercised and stays working.
{args: "flink environment create default-failure --kubernetes-namespace default-staging", fixture: "flink/environment/create-failure.golden", exitCode: 1},
{args: "flink environment create default --kubernetes-namespace default-staging", fixture: "flink/environment/create-existing.golden", exitCode: 1},
{args: "flink environment create default", fixture: "flink/environment/create-no-namespace.golden", exitCode: 1},
{args: "flink environment create default-2 --kubernetes-namespace default-staging --statement-defaults '{\"config-overrides\":{\"key\":\"value\"}}'", fixture: "flink/environment/create-statement-defaults-invalid.golden", exitCode: 1},
// success with application, statement and compute pool defaults
{args: "flink environment create default-2" +
" --defaults test/fixtures/input/flink/environment/application-defaults.json" +
" --statement-defaults test/fixtures/input/flink/environment/statement-defaults.json" +
" --compute-pool-defaults test/fixtures/input/flink/environment/compute-pool-defaults.json" +
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+152
to
+163
| func decodeStrictJson(data []byte, out any) error { | ||
| decoder := json.NewDecoder(bytes.NewReader(data)) | ||
| decoder.DisallowUnknownFields() | ||
| return decoder.Decode(out) | ||
| } | ||
|
|
||
| // decodeStrictYaml is the YAML counterpart of decodeStrictJson. | ||
| func decodeStrictYaml(data []byte, out any) error { | ||
| decoder := yaml.NewDecoder(bytes.NewReader(data)) | ||
| decoder.KnownFields(true) | ||
| return decoder.Decode(out) | ||
| } |
Contributor
Author
Manual CLI validationLocal CMF ( Before — released
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Release Notes
Breaking Changes
New Features
Bug Fixes
confluent flink environment create/updatenow reject a mis-shaped--statement-defaultsvalue with a clear error instead of silently ignoring it and reporting success.Checklist
Whatsection below whether this PR applies to Confluent Cloud, Confluent Platform, or both.Test & Reviewsection below.Blast Radiussection below.What
Confluent Platform (CP Flink / CMF on-prem).
--statement-defaultswas parsed with plainjson.Unmarshal, which silently drops keys that don't map to a field. A wrong-shape value (e.g.{"config-overrides":{...}}) set nothing yet exited 0 -- and, since update is a full replace, cleared any existing statement defaults.parseDefaultsAsGenericTypenow decodes strictly (DisallowUnknownFields/ YAMLKnownFields). Safe for the map-based--defaults/--compute-pool-defaults(a map has no unknown fields), so only the typed--statement-defaultspath is tightened. Flag help now documents the expected shape.Blast Radius
--statement-defaultsvalue with unknown/mis-nested keys now errors (non-zero exit) instead of silently succeeding -- the intended correction; automation relying on the old silent-drop will now surface an error.--defaultsand--compute-pool-defaultsare unaffected.environment updateis a full replace, so a partial update also clears the other defaults (tracked separately).References
Test & Review
Environment: local CMF
cmf-app 2.4-SNAPSHOT(HTTP,--cmf.k8s.enabled=false); CLI built from this branch vs releasedconfluent v4.54.0.Manual CLI validation: attached in the comment below.