Bump cmf-sdk-go v0.0.5 to v0.0.6 and fix breaking changes#3320
Bump cmf-sdk-go v0.0.5 to v0.0.6 and fix breaking changes#3320Paras Negi (paras-negi-flink) wants to merge 6 commits intomainfrom
Conversation
ComputePool.Status changed from *ComputePoolStatus to
*map[string]map[string]interface{} and PostEnvironment.Name changed
from string to *string. This updates all call sites to use SDK
getters/setters, adds extractComputePoolPhase() helper for the
generic status map, adds empty-name guards to environment create/update,
and updates test handler and golden files accordingly.
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
|
🎉 All Contributor License Agreements have been signed. Ready to merge. |
There was a problem hiding this comment.
Pull request overview
Updates the CLI to accommodate breaking changes introduced in cmf-sdk-go v0.0.6, specifically around PostEnvironment.Name becoming a pointer and ComputePool.Status becoming a generic nested map. This keeps on-prem Flink environment and compute-pool commands working and aligns test fixtures with the updated API shapes.
Changes:
- Bump
github.com/confluentinc/cmf-sdk-gofromv0.0.5tov0.0.6(go.mod/go.sum). - Update environment create/update to use SDK getters/setters and add empty-name validation in the CMF REST client.
- Add
extractComputePoolPhase()to read compute pool phase from the new generic status map, and update on-prem compute-pool commands + test server + golden fixtures accordingly.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
pkg/flink/cmf_rest_client.go |
Uses PostEnvironment.GetName() and validates non-empty environment names for create/update. |
internal/flink/command_environment_create.go |
Uses postEnvironment.SetName() to match pointer-based SDK field. |
internal/flink/command_environment_update.go |
Uses postEnvironment.SetName() to match pointer-based SDK field. |
internal/flink/command_compute_pool.go |
Adds extractComputePoolPhase() and updates compute-pool serialization to use it. |
internal/flink/command_compute_pool_list_onprem.go |
Uses extractComputePoolPhase() for list output. |
internal/flink/command_compute_pool_describe_onprem.go |
Uses extractComputePoolPhase() for describe output. |
internal/flink/command_compute_pool_create_onprem.go |
Uses extractComputePoolPhase() for create output. |
test/test-server/flink_onprem_handler.go |
Updates compute-pool and environment handlers to emit/consume new SDK shapes. |
test/fixtures/output/flink/compute-pool/list-json.golden |
Updates expected JSON (clusterSpec now {} instead of null). |
test/fixtures/output/flink/compute-pool/describe-success-json.golden |
Updates expected JSON (clusterSpec now {} instead of null). |
go.mod |
Bumps cmf-sdk-go dependency version. |
go.sum |
Updates checksums for cmf-sdk-go v0.0.6. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The JSON marshal/unmarshal fallback path can never succeed given the SDK
type map[string]map[string]interface{} — marshaling always produces
nested objects which cannot unmarshal into map[string]string. Remove the
unreachable code and stderr warnings to keep the function side-effect free.
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Paras Negi (paras-negi-flink)
requested review from
Milind L (milindl) and
Robert Metzger (rmetzger)
SDK v0.0.6 types ComputePool.Status as *map[string]map[string]interface{}
but the API returns a flat object like {"phase":"RUNNING"}, causing the
SDK's Execute() to fail on valid 200 responses. Add fallback helpers that
re-parse the buffered response body when this occurs.
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
The previous commit rewrote createComputePool to a raw map and explicitly
set clusterSpec to an empty map, which serialized as {} in golden JSON
outputs (vs the prior null). Dropping the clusterSpec key from the mock
leaves ComputePool.Spec.ClusterSpec at its zero value (nil map), which
serializes as null — matching the original golden files.
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
The fallback pattern (re-read buffered response body, attempt manual parse, re-buffer on failure) was duplicated across CreateComputePool, DescribeComputePool, and ListComputePools. Extract readFallbackBody and a generic tryComputePoolFallback helper so each call site collapses to a three-line check, and the failure/re-buffer discipline lives in one place. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
|
|
What caused breaking change 2? |
| // extractComputePoolPhase extracts the phase string from the generic status map. | ||
| // ComputePool.Status is typed as *map[string]map[string]interface{} in SDK v0.0.6. | ||
| func extractComputePoolPhase(pool cmfsdk.ComputePool) string { | ||
| if pool.Status == nil { | ||
| return "" | ||
| } | ||
| status := pool.GetStatus() | ||
| if phaseMap, ok := status["phase"]; ok { | ||
| if value, ok := phaseMap["value"].(string); ok { | ||
| return value | ||
| } | ||
| } | ||
| return "" | ||
| } |
There was a problem hiding this comment.
Wouldn't it make sense to move this utility into the SDK?
There was a problem hiding this comment.
My understanding is that we don't do human code push in cmf-go-sdk, but if we want to : Adds a custom UnmarshalJSON method on the ComputePool type (in a new file v1/model_compute_pool_custom.go ) seems to be the better fix, which completely removes "custom unmarshaling with json.RawMessage embedding" from cli repo
There was a problem hiding this comment.
My understanding is that we don't do human code push in cmf-go-sdk, but if we want to
My only worry about human pushes in the sdk repo is that the code generator is overwriting human written code / utils.
But when we started the go sdk repo, I actually had additional, non generated code for shared utilities in mind.
| } | ||
|
|
||
| if sdkComputePool.Status != nil { | ||
| if phase := extractComputePoolPhase(sdkComputePool); phase != "" { |
There was a problem hiding this comment.
Can we always call this method, also when the CLI runs against older CMF versions? Probably yes? because the response has not changed, only the SDK.
| } | ||
|
|
||
| // readFallbackBody returns the HTTP response body when the SDK's Execute() returned | ||
| // an error despite a successful 2xx status — the ComputePool deserialization bug. |
There was a problem hiding this comment.
What is "the ComputePool deserialization bug"? I don't think this is a bug?
It's just that the SDK's interfaces have changed
|
narrow the error signature. The v0.0.6 bug produces a specific var typeErr *json.UnmarshalTypeError
if !errors.As(err, &typeErr) || typeErr.Field != "status" {
return nil // not the bug we're working around
}Brittle if the SDK changes error wrapping, but explicitly scoped to the known bug. Any other decode failure now surfaces correctly. |
What
Bumps
cmf-sdk-gofrom v0.0.5 → v0.0.6 and handles the two breaking changes that ship with it. Applies to Confluent Platform on-prem only.Breaking change 1:
ComputePool.StatustypeThe generator now produces
*map[string]map[string]interface{}forComputePool.Status(caused by an inline OpenAPI schema withadditionalProperties: trueinstead of a$reftoComputePoolStatus). The real CMF API returns a flat status like{"phase":"RUNNING","message":null}, whichjson.Unmarshalcannot fit into the declared innermap[string]interface{}, so the SDK'sExecute()fails on every valid 200 response — breakingcompute-pool list/describe/create.Fix: a fallback in
pkg/flink/cmf_rest_client.go. When the SDK returns an error but the HTTP status is 2xx, we re-read the buffered response body and parse it ourselves:unmarshalComputePoolembedscmfsdk.ComputePooland shadowsStatuswithjson.RawMessage. Everything except status unmarshals via the default path; status is captured as raw JSON, the phase is extracted, and re-encoded into the nested format ({"phase":{"value":"RUNNING"}}) that the existingextractComputePoolPhasehelper expects.unmarshalComputePoolsPagedoes the same for paginated list responses.parseSdkErrorcan still produce a useful error message.Breaking change 2:
PostEnvironment.NametypeChanged from
stringto*stringwithGetName()/SetName()accessors. Call sites incommand_environment_create.goandcommand_environment_update.gomigrated, with nil-name guards inCreateEnvironment/UpdateEnvironment.Test handler rewrite (forced, not optional)
test/test-server/flink_onprem_handler.gohad to be modified because two types it used no longer compile against v0.0.6:cmfsdk.ComputePoolStatus{Phase: phase}— this struct type no longer exists in v0.0.6 (replaced by the generic map).environment.Namereads —Nameis now*string, so all reads were migrated toenvironment.GetName().While rewriting
createComputePool, output is changed to a rawmap[string]interface{}with flat status ("phase": "RUNNING") so the mock matches the real CMF API response. This ensures the CLI's fallback path is actually exercised in CI rather than being dead code that only runs in production.Scope
pkg/flink/cmf_rest_client.go, isolated to the 3 compute-pool methodsCmfRestClientnull→{}only (cosmetic, from test mock defaulting)Release Notes
Breaking Changes
New Features
Bug Fixes
Checklist
Whatsection below whether this PR applies to Confluent Cloud, Confluent Platform, or both.Test & Reviewsection below.Blast Radiussection below.Blast Radius
Scope is limited to on-prem Flink compute pool and environment commands; Confluent Cloud commands (which use ccloud-sdk-go-v2, not cmf-sdk-go) are completely unaffected.
Tests