[PATCH] Implement list-vms command

[Omair Majid]

On 04/12/2012 10:28 AM, Roman Kennke wrote:
> This patch implements the first useful command, list-vms. This command
> prints out a list of currently monitored VMs.
> 
> A few implementation notes:
> 
> - I added a generic attribute interface to ApplicationContext to store
> and retrieve other objects than we have in the API. Motivation for this
> is that some modules will require different things in the AppContext
> than others. Access to this is done by an enum, and should go through an
> accessor-helper-class that does the casting.

Not sure how I feel about making it easier to add more global state.

> - The DB connection and ApplicationContext is set up in AppContextSetup
> class (this will be reused by other commands).
> - Argument parsing is done by JOpt-Simple.
> 
> For an example of how this looks like, see here:
> http://rkennke.wordpress.com/2012/04/12/thermostat-cli/
> 

Thanks for that link which shows the command active. Looking at this, I
wonder if the adding more commands like this is the right way. The
existing commands do major things (start agent, start db, (TODO: start
gui client)), while this one is a "small" command. Seems to me like we
need a similar major command. How about changing it to something like
this instead:

thermostat query list-vms

What do you think?

> There are situations that are not yet handled well:
> - when the DB is not running
> - when the agent is not running (missing host-info collection)
> 
> Both result in not very useful (for ordinary users) stacktraces. This
> needs to be addressed in another iteration.
> 
> Ok to go in?

More comments below.

> diff --git a/tools/src/main/java/com/redhat/thermostat/tools/cli/AppContextSetup.java b/tools/src/main/java/com/redhat/thermostat/tools/cli/AppContextSetup.java
> new file mode 100644
> --- /dev/null
> +++ b/tools/src/main/java/com/redhat/thermostat/tools/cli/AppContextSetup.java

> +class AppContextSetup {
> +
> +    void setupAppContext(String dbUrl) {
> +        StartupConfiguration config = new ConnectionConfiguration(dbUrl);
> +        
> +        ConnectionProvider connProv = new MongoConnectionProvider(config);
> +        DAOFactory daoFactory = new MongoDAOFactory(connProv);
> +        Connection connection = daoFactory.getConnection();
> +        connection.connect();
> +        ApplicationContext.getInstance().setDAOFactory(daoFactory);
> +

This bit here looks like a duplicate of/similar to what's in the gui
client's main. Perhaps this code can be shared?

> diff --git a/tools/src/main/java/com/redhat/thermostat/tools/cli/CLIAppContextUtils.java b/tools/src/main/java/com/redhat/thermostat/tools/cli/CLIAppContextUtils.java
> new file mode 100644
> --- /dev/null
> +++ b/tools/src/main/java/com/redhat/thermostat/tools/cli/CLIAppContextUtils.java

> +public class CLIAppContextUtils {
> +
> +    public static AppContextSetup getAppContextSetup() {
> +        ApplicationContext ctx = ApplicationContext.getInstance();
> +        AppContextSetup setup = (AppContextSetup) ctx.getAttribute(CLIAppContext.SETUP);
> +        if (setup == null) {
> +            setup = new AppContextSetup();
> +            setAppContextSetup(setup);
> +        }
> +        return setup;
> +    }
> +
> +    public static void setAppContextSetup(AppContextSetup setup) {
> +        ApplicationContext ctx = ApplicationContext.getInstance();
> +        ctx.setAttribute(CLIAppContext.SETUP, setup);
> +    }
> +

I don't see why the AppContextSetup has to be stored as an attribute.
Can't we just construct AppContextSetup once when we need it?

> diff --git a/tools/src/main/java/com/redhat/thermostat/tools/cli/ListVMsCommand.java b/tools/src/main/java/com/redhat/thermostat/tools/cli/ListVMsCommand.java
> new file mode 100644
> --- /dev/null
> +++ b/tools/src/main/java/com/redhat/thermostat/tools/cli/ListVMsCommand.java

To me, this is as much a client as the gui client. Perhaps this should
be part of the client module instead of the tools module?

> +public class ListVMsCommand implements Command {
> +
> +    @Override
> +    public void run(CommandContext ctx) throws CommandException {
> +
> +        String dbUrl = parseArguments(ctx);
> +        
> +        CLIAppContextUtils.getAppContextSetup().setupAppContext(dbUrl);

I found this line really hard to parse. Can we do something about it?

> +    @Override
> +    public String getName() {
> +        return "list-vms";
> +    }
> +
> +    @Override
> +    public String getDescription() {
> +        return "list-vms";
> +    }
> +
> +    @Override
> +    public String getUsage() {
> +        return "list-vms";
> +    }

Can you please add more descriptive versions of this text?

Cheers,
Omair