-
Notifications
You must be signed in to change notification settings - Fork 30
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
feat: Add 'Create Namespace' command to CLI #179
Conversation
cmd/iceberg/main.go
Outdated
@@ -70,6 +71,7 @@ type Config struct { | |||
Uuid bool `docopt:"uuid"` | |||
Location bool `docopt:"location"` | |||
Props bool `docopt:"properties"` | |||
Create bool `docopt:"create"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any way we could add a test of some kind for this? perhaps a dummy category type to confirm we call the function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zeroshade I can create test to call main
function with test arguments:
func TestCommands(t *testing.T) {
os.Args = []string{"iceberg", "create", "namespace", "public"}
main()
}
But I don't know how could I mock and verify call on catalog
.
Otherwise it just tries to connect to real catalog and fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair enough. Something we should look into in the future being a way to mock the catalog stuff to create a framework for unit testing the CLI. Don't think we should prevent this getting merged waiting for that though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zeroshade Resolved conflicts after pulling latest main
, but actually there is not much to merge as similar changes in CLI were applied by #173 : just remove duplicated "create" command and unsupported error for "create table".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this @alex-kar and thanks for the review @zeroshade
As
ListNamespaces
already implemented inRestCatalog
, we could add that command to CLI.