Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add delete topic command #100

Closed
wants to merge 3 commits into from
Closed

add delete topic command #100

wants to merge 3 commits into from

Conversation

rislah
Copy link

@rislah rislah commented Oct 6, 2022

Repl has been set to read only mode to prevent accidental deletes, configurable through CLI flag.

Delete operation must be confirmed with a yes/no

* repl is set to read-only by default

* update README
@rislah rislah requested a review from a team as a code owner October 6, 2022 06:21
Copy link
Contributor

@hhahn-tw hhahn-tw left a comment

Choose a reason for hiding this comment

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

Hello, and thank you for your pull request. I'm sure many will be happy to be able to delete topics via topicctl and we appreciate your contribution.

After some deliberation, we've come to a consensus that the changes to make the repl mode write-capable do not fit the design goals at this time. It is something to consider in the future but it would make more sense as a part of a larger comprehensive overhaul rather than a singleton use case.

Could you resubmit without the repl changes?

@@ -484,6 +484,28 @@ func (c *CLIRunner) GetTopics(ctx context.Context, full bool) error {
return nil
}

// DeleteTopic deletes a single topic.
func (c *CLIRunner) DeleteTopic(ctx context.Context, topic string) error {
confirm, err := apply.Confirm(fmt.Sprintf("Delete topic \"%s\"", topic), false)
Copy link
Contributor

Choose a reason for hiding this comment

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

This asks for confirmation without verifying it's an existing topic which can be confusing. In the interest of taking every step to confirm safe operation, can you add a check here that a topic of the given name exists prior to asking user confirmation?

Long: strings.Join(
[]string{
"Deletes instances of a particular type.",
"Supported types currently include: topic.",
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see any other obvious resource types a delete function would be applicable to - did you have something in mind?

@petedannemann petedannemann added the stale Waiting for response from a contributor label Jul 11, 2023
@petedannemann
Copy link
Contributor

@rislah did you get a chance to review @hhahn-tw 's comments?

@petedannemann petedannemann mentioned this pull request Jul 11, 2023
@petedannemann petedannemann mentioned this pull request Nov 9, 2023
@petedannemann
Copy link
Contributor

Superseding this with #163 due to inactivity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Waiting for response from a contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants