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

[#5963] feat(client): added delete cli command model #6099

Merged
merged 6 commits into from
Jan 5, 2025

Conversation

VigneshSK17
Copy link
Contributor

What changes were proposed in this pull request?

The delete command is one of the commands suggested by @justinmclean as part of adding Model entity support for the CLI.

Why are the changes needed?

To add delete functionality for a Model using the CLI

Improvement: #5963 (NOTE: Create command is redundant with addition of Register command)

Does this PR introduce any user-facing change?

Yes.

The delete command for a model was added.

How was this patch tested?

Unit tests were added for Model CLI support and ran successfully for the delete command, along with CI tests in forked repository

@VigneshSK17 VigneshSK17 changed the title [#5962] feat(client): added delet cli command model [#5963] feat(client): added delete cli command model Jan 5, 2025
@VigneshSK17
Copy link
Contributor Author

@justinmclean When you find the time, could you please review this?

Thanks.

{
boolean force = line.hasOption(GravitinoOptions.FORCE);
newDeleteModel(url, ignore, force, metalake, catalog, schema, model).handle();
break;
Copy link
Member

Choose a reason for hiding this comment

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

very minor, but keep break as last line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did update to fix this, but I'm curious if you mean that break should be the last line of a case clause, even if there's braces around the code inside the case clause?

@VigneshSK17
Copy link
Contributor Author

Made the slight changes to the docs as mentioned above.

Copy link
Member

@justinmclean justinmclean left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this.

@justinmclean justinmclean merged commit 86ef6e8 into apache:main Jan 5, 2025
27 checks passed
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