Skip to content

KFS-362 Fix deleteTopics usage in confluent ksql cluster delete cmd#1536

Merged
Martin Tang (MarTango) merged 1 commit intomainfrom
bugfix/ksql-cluster-delete
Nov 22, 2022
Merged

KFS-362 Fix deleteTopics usage in confluent ksql cluster delete cmd#1536
Martin Tang (MarTango) merged 1 commit intomainfrom
bugfix/ksql-cluster-delete

Conversation

@MarTango
Copy link
Contributor

Fixes two issues:

  • We were calling c.deleteTopics(req), but req.Endpoint is nil.
  • We had go if !provisioningFailed && err != nil { } which is always false.

Checklist

  1. [CRUCIAL] Is the change for CP or CCloud functionalities that are already live in prod?
    • yes: ok

What

These fixes were already applied to the v3 branch. Pulling them into main in this PR

References

#1371

Fixes two issues:
- We were calling `c.deleteTopics(req)`, but `req.Endpoint` is `nil`.
- We had
  ```go
  if !provisioningFailed && err != nil {
  }
  ```
  which is always false.
@MarTango Martin Tang (MarTango) requested a review from a team as a code owner November 21, 2022 16:27
Comment on lines +85 to 87
client := sling.New().Client(oauth2.NewClient(context.Background(), ts)).Base(endpoint)
request := map[string][]string{"deleteTopicList": {".*"}}
response, err := client.Post("/ksql/terminate").BodyJSON(&request).ReceiveSuccess(nil)

Choose a reason for hiding this comment

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

Remind me why we're not using an SDK (such as ccloud-sdk-go-v2) here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

KSQL itself doesn't have an official (go) SDK (I'm not sure why - probably because the API isn't stable yet), which is why we use a generic rest-client.. maybe?

There does seem to be an unofficial go client, but integrating it here is outside the scope of this bugfix.

Copy link

@brianstrauch Brian Strauch (brianstrauch) Nov 22, 2022

Choose a reason for hiding this comment

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

Ah, right. I think we talked about this in the past, and you or someone else on your team opened a ticket to make a ksql-rest-sdk-go repo, similar to https://github.com/confluentinc/kafka-rest-sdk-go. It would be great if that could be prioritized since the current method is difficult to test and impossible to debug.

@MarTango Martin Tang (MarTango) merged commit baabe70 into main Nov 22, 2022
@MarTango Martin Tang (MarTango) deleted the bugfix/ksql-cluster-delete branch November 22, 2022 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants