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

ARTEMIS-670 Adding destination creation and deletion cli command #705

Closed
wants to merge 1 commit into from

Conversation

gaohoward
Copy link
Contributor

No description provided.

@gaohoward
Copy link
Contributor Author

seems the failures are unrelated to this commit. Looking into it now.

@gaohoward
Copy link
Contributor Author

They all pass on my local env so those are not related to the commit.

@clebertsuconic
Copy link
Contributor

I think this gets a bit confused for users...

Why don't you add the following commands:

create-queue
create-topic
delete-queue
delete-topic

You can have abstract classes for what is common among the classes. It will be the same implementation almost, but the user would have more control on what he wants to be done.

The way this is structure, you are using a create-queue command to delete it. I don't think it's too intuitive to the user.

@gaohoward
Copy link
Contributor Author

ok, sounds good. I'll do it. Thanks Clebert.

@clebertsuconic
Copy link
Contributor

@howardgao one thing you could do.. is to use sub-commands... look at data...

you have data print, data exp... etc

you could have queue create, queue add, ... etc...

even do ./artemis help queue to list the sub commands

@gaohoward
Copy link
Contributor Author

I think using sub commands is not better than single commands as you previously suggested.

@clebertsuconic
Copy link
Contributor

@gaohoward It is the same thing... just an organizational thingy.

do separate commands for now. I can do it as a little tweak later.

@gaohoward
Copy link
Contributor Author

@clebertsuconic ok done.

@clebertsuconic
Copy link
Contributor

@gaohoward when I wrote the delete-topic up here was just as an example of separating the commands. I didn't mean to be literal. Do you actually need to differenticate delete-topic from delete-queue or it could just be delete?

@johnament
Copy link
Contributor

I personally prefer (create|delete)-(topic|queue). This doesn't mean the others shouldn't be there, but they can be aliased. This way either could work.

@gaohoward
Copy link
Contributor Author

@clebertsuconic yeah we can combine the delete-topic and delete-queue, but having them in separate commands is not bad. I really don't have any preferences of one over the other.

@clebertsuconic
Copy link
Contributor

@johnament I liked the idea... we can't use create though on the CLI as it's used to create a broker..

although we could use it with a group:

./artemis dest create --queue ...etc etc
./artemis dest delete --queue | topic

@jbertram
Copy link
Contributor

jbertram commented Aug 9, 2016

One concern I have is that I can't tell by the names if these commands are operating on JMS resources or core resources. Obviously there's no such thing as a core topic so I assume that the topic-related commands are operating on a JMS topic. However, core has a queue and JMS has a queue. If the commands are dealing with JMS resources then I think the might need to reflect that to avoid confusion.

@clebertsuconic
Copy link
Contributor

@jbertram maybe we could call the group jms
artemis jms create --queue(default) | --topic --name name
artemis jms delete --name name (I'm not convinced we need --queue or --topic here?, we could figure out from the search?)

then we could have

artemis core create
artemis core delete

?

@gaohoward
Copy link
Contributor Author

@jbertram by default create-queue creates jms queues, if you want create a core queue, use --core option, like

artemis create-queue --core --name corequeue1 (optionally you can also give a --address option)

delete-queue has --core option too

@gaohoward
Copy link
Contributor Author

@clebertsuconic that kinda goes back to my original impl, which combine all the functionalities into one command, i.e.

./artemis destination create|delete --name myqueue1 --core

which is not so good in that some options are irrelevant for certain operations, causing confusion. for example delete a queue is just need a name, the filter, jndi bindings, etc are not relevant. If people do

./artemis help destination

it will print all the options together. Having separated commands can reduce the confusion, because each command has only relevant options.

@clebertsuconic
Copy link
Contributor

@gaohoward It's different.. you had the behaviour specified by properties... which was the confusing part.

Grouping is different... you still have separated commands, and aggregate them on the help. just like data.

@gaohoward
Copy link
Contributor Author

@clebertsuconic ok looks like the sub-command style (like data command) has no objection. I'll go for it. Thanks

@gaohoward
Copy link
Contributor Author

@clebertsuconic I've update my commit. pls review. Now if you run help it will print look like this:
]$ ./artemis help destination
NAME
artemis destination - Destination tools group (create|delete) (example
./artemis destination create)

SYNOPSIS
artemis destination
artemis destination create [--durable] --name [--type ]
[--verbose] [--url ] [--password ]
[--address

] [--bindings ] [--filter ]
[--user ]
artemis destination delete --name [--removeConsumers]
[--type ] [--verbose] [--url ]
[--password ] [--user ]

COMMANDS
With no arguments, Display help information

    create
        create a queue or topic

        With --durable option, whether the queue is durable or not (default
        false)

        With --name option, destination name

        With --type option, type of destination to be created (one of
        jms-queue, topic and core-queue, default jms-queue

        With --verbose option, Adds more information on the execution

        With --url option, URL towards the broker. (default:
        tcp://localhost:61616)

        With --password option, Password used to connect

        With --address option, address of the core queue (default queue's
        name)

        With --bindings option, comma separated jndi binding names (default
        null)

        With --filter option, queue's filter string (default null)

        With --user option, User used to connect

    delete
        delete a queue or topic

        With --name option, destination name

        With --removeConsumers option, whether deleting destination with
        consumers or not (default false)

        With --type option, type of destination to be created (one of
        jms-queue, topic and core-queue, default jms-queue

        With --verbose option, Adds more information on the execution

        With --url option, URL towards the broker. (default:
        tcp://localhost:61616)

        With --password option, Password used to connect

        With --user option, User used to connect

@clebertsuconic
Copy link
Contributor

it looks cool on the inline help 👍

@asfgit asfgit closed this in d07e60d Aug 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants