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

[CLI] Delete commands should support multiple targets #2427

Closed
tjtelan opened this issue Jun 20, 2022 · 4 comments
Closed

[CLI] Delete commands should support multiple targets #2427

tjtelan opened this issue Jun 20, 2022 · 4 comments
Labels
CLI good first issue Good for newcomers help wanted Good issue for community involvement UX/Usability
Milestone

Comments

@tjtelan
Copy link
Contributor

tjtelan commented Jun 20, 2022

We should allow the fluvio [topic, smart-module, connectors, etc...] delete to accept multiple names.

The intended outcome is that each of the names listed get deleted, as opposed to 1 command per resource.

Unless there are strong feelings for a different default behavior, in the event of an error, we should return an error to stderr, and exit on the first encounter. But if we provide a flag, something like --force or --continue-on-error, then cycle through all the names.

@tjtelan tjtelan added help wanted Good issue for community involvement good first issue Good for newcomers CLI UX/Usability labels Jun 20, 2022
@ajhunyady
Copy link
Contributor

What if a SmartModule is referenced in a Connector? I believe, we should prevent it from being deleted, otherwise, the connector may unexpectedly stop working.

@tjtelan
Copy link
Contributor Author

tjtelan commented Jun 22, 2022

@ajhunyady I don't think it is the CLI's responsibility to check on the SmartModules in use by connectors. If that workflow is available now, then we should prevent it on the cluster while it is servicing the request.

The point of this issue is more about making it easier to send multiple requests from the CLI with a single command, by looping over a list of names instead of exactly one.

@tomuxmon
Copy link
Contributor

Hey, according to clap you can just Use Vec<T> and add required = true to disallow zero-length Vec.
Would look something like this:

#[derive(Debug, Parser)]
pub struct DeleteTopicOpt {
    /// The name of the Topic to delete
    #[clap(value_name = "name", required = true)]
    topics: Vec<String>,
}

impl DeleteTopicOpt {
    pub async fn process(self, fluvio: &Fluvio) -> Result<()> {
        let admin = fluvio.admin().await;
        for topic in self.topics.iter() {
            debug!("deleting topic: {}", topic);
            admin.delete::<TopicSpec, _>(topic).await?;
            println!("topic \"{}\" deleted", topic);
        }
        Ok(())
    }
}

I can prepare a pull request and see what it looks like.

@sehz sehz added this to the 0.9.33 milestone Jul 29, 2022
bors bot pushed a commit that referenced this issue Jul 29, 2022
…s. (#2518)

Solves #2427

Allows delete of multiple:
- connectors, 
- smart modules 
- topics

Left out the cluster delete. Does not seem suitable to be able to delete multiple clusters at once.

To be fixed:
- [x] continue-on-error instead of ignore-error
- [x] changelog entry
- [x] update on subcommand description
- [x] friendly error message with `CliError::from(error)`
- [x] split `CliError::print`
- [x] separate error type for multi delete fail


Co-authored-by: tomuxmon <tomas.sql@protonmail.com>
@sehz
Copy link
Contributor

sehz commented Aug 10, 2022

done

@sehz sehz closed this as completed Aug 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLI good first issue Good for newcomers help wanted Good issue for community involvement UX/Usability
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants