Add --kafka-cluster flag to usm connect register - #3434
Open
Arman Garg (FlamedHunter) wants to merge 2 commits into
Open
Add --kafka-cluster flag to usm connect register#3434Arman Garg (FlamedHunter) wants to merge 2 commits into
--kafka-cluster flag to usm connect register#3434Arman Garg (FlamedHunter) wants to merge 2 commits into
Conversation
|
🎉 All Contributor License Agreements have been signed. Ready to merge. |
There was a problem hiding this comment.
Pull request overview
Adds support for registering a USM Connect cluster using a new --kafka-cluster flag (in addition to the existing --confluent-platform-kafka-cluster), including updated help text and integration fixtures.
Changes:
- Add
--kafka-clusterflag tousm connect register, making the two Kafka cluster flags mutually exclusive and requiring one of them. - Introduce a “hybrid” output shape for the
--kafka-clusterflow. - Update integration tests and golden outputs to reflect the new flag behavior and validation errors.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
internal/unified-stream-manager/command_connect_register.go |
Adds the new flag, updates examples, and adjusts flag validation/registration flow. |
internal/unified-stream-manager/command_connect.go |
Adds a new output struct + printer used by the --kafka-cluster registration path. |
test/usm_test.go |
Extends integration test coverage for new flag validation; currently includes commented-out success-path cases. |
test/fixtures/output/unified-stream-manager/connect/register-help.golden |
Updates help output to include the new flag and example. |
test/fixtures/output/unified-stream-manager/connect/create-fail-paired-flag-missing.golden |
Updates usage/help output shown on paired-flag validation error. |
test/fixtures/output/unified-stream-manager/connect/create-fail-mutually-exclusive-kafka-flags.golden |
Adds fixture for mutually-exclusive Kafka flag error. |
test/fixtures/output/unified-stream-manager/connect/create-fail-missing-kafka-flag.golden |
Adds fixture for “one required” Kafka flag validation error. |
test/fixtures/output/unified-stream-manager/connect/create-cp-kafka.golden |
Adds fixture for the --kafka-cluster success-path output currently exercised by tests. |
Suppressed comments (1)
internal/unified-stream-manager/command_connect_register.go:69
- The --kafka-cluster path still flows through the on-prem→cloud Kafka ID mapping and the "not found" logging. For cloud IDs like "lkc-...", this will always miss the map, log a misleading error on success, and can even fail early if ListUsmKafkaClusters requires permissions (even though the mapping is irrelevant for --kafka-cluster).
if cmd.Flags().Changed("kafka-cluster") {
kafkaClusterId, err = cmd.Flags().GetString("kafka-cluster")
if err != nil {
return err
}
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
33
to
+34
| cmd.Flags().String("confluent-platform-kafka-cluster", "", "The ID of the metadata Kafka cluster for the Connect Cluster.") | ||
| cmd.Flags().String("kafka-cluster", "", `The ID of the metadata Kafka cluster for the Connect Cluster.`) |
Comment on lines
+43
to
+46
| //{args: "usm connect register connect-group-xyz123 --kafka-cluster lkc-abc123 --cloud aws --region us-east-1", fixture: "unified-stream-manager/connect/create-cloud-kafka.golden"}, | ||
| //{args: "usm connect register connect-group-xyz123 --kafka-cluster lkc-abc123 --cloud aws --region us-east-1 -o json", fixture: "unified-stream-manager/connect/create-cloud-kafka-json.golden"}, | ||
| {args: "usm connect register connect-group-xyz123 --kafka-cluster 4k0R9d1GTS5tI9f4Y2xZ0Q --cloud aws --region us-east-1", fixture: "unified-stream-manager/connect/create-cp-kafka.golden"}, | ||
| {args: "usm connect register connect-group-xyz123 --confluent-platform-kafka-cluster 4k0R9d1GTS5tI9f4Y2xZ0Q --kafka-cluster lkc-abc123 --cloud aws --region us-east-1", fixture: "unified-stream-manager/connect/create-fail-mutually-exclusive-kafka-flags.golden", exitCode: 1}, |
|
| type connectHybridOut struct { | ||
| Id string `human:"ID" serialized:"id"` | ||
| ConnectCluster string `human:"Connect Cluster" serialized:"connect_cluster"` | ||
| USMKafkaClusterId string `human:"USM Kafka Cluster ID" serialized:"usm_kafka_cluster_id"` |
There was a problem hiding this comment.
Can we check with the cli team is it is okay to remove this flag altogether?
or is the recommendation to deprecate this first?
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
--kafka-clusterflag toconfluent unified-stream-manager connect register, accepting either a Confluent Platform Kafka cluster ID or a Confluent Cloud Kafka cluster ID (lkc-...), as an alternative to--confluent-platform-kafka-cluster.Bug Fixes
Checklist
Whatsection below whether this PR applies to Confluent Cloud, Confluent Platform, or both.Test & Reviewsection below.Blast Radiussection below.What
--kafka-clusterflag toconfluent unified-stream-manager connect register, accepting either a Confluent Platform Kafka cluster ID or a Confluent Cloud Kafka cluster ID (lkc-...), as an alternative to--confluent-platform-kafka-cluster.--confluent-platform-kafka-cluster's behavior and response shape are unchanged.--kafka-clusterregistrations print a new, simplified response (a single "Kafka Cluster" field) containing the value passed to the flag as-is. TheCP-to-USMresolution lookup still runs internally but its result isn't surfaced in this response.kafka_cluster_idfield on the existingusm/v1/connect-clusterscreate request.Blast Radius
References
Test & Review
For cloud Kafka cluster (registered on USM as hybrid)
Existing behaviour
Using new flag (
kafka-cluster)For cloud Kafka cluster
Existing behaviour
Using new flag (
kafka-cluster)For on-prem Kafka Cluster
Existing behaviour
Using new flag (
kafka-cluster)