juju offers command takes a model arg #7699

Merged
merged 2 commits into from Aug 3, 2017

Conversation

Projects
None yet
3 participants
Owner

wallyworld commented Aug 2, 2017

Description of change

The "juju offers" command is now a model command so that you can use -m to tell it to operate on a different model to the current one.

QA steps

Run up a cmr scenario
switch to model hosting offers
juju offers -> see results
switch to another model
juju offers -m -> results
juju offers -m / -> results

ISTM it would be more sensible to make it a model command, and use the usual -m flag. No need for special arg handling then.

Owner

wallyworld commented Aug 2, 2017

list-offers is now a model command

- return errors.Annotate(err, "cannot load current model")
+ return errors.Trace(err)
+ }
+ if !jujuclient.IsQualifiedModelName(modelName) {
@axw

axw Aug 3, 2017

Member

There's a lot of duplication here with what's done by the modelcmd code. This isn't the appropriate place for this code.

Can you please split the ModelCommandBase.NewAPIRoot method into two, so everything before the call to c.newAPIRoot is in a new method. Something like:

func (c *ModelCommand) ModelDetails() (*jujuclient.ModelDetails, error) {
    modelName, err := c.ModelName()
    if err != nil {
        return nil, errors.Trace(err)
    }
    controllerName, err := c.ControllerName()
    if err != nil {
        return nil, errors.Trace(err)
    }
    if modelName == "" {
        return nil, errors.Trace(ErrNoModelSpecified)
    }
    details, err = c.store.ModelByName(controllerName, modelName)
    if err != nil {
        if !errors.IsNotFound(err) {
                return nil, errors.Trace(err)
        }
        // The model isn't known locally, so query the models
        // available in the controller, and cache them locally.
        if err := c.RefreshModels(c.store, controllerName); err != nil {
                return nil, errors.Annotate(err, "refreshing models")
        }
        details, err = c.store.ModelByName(controllerName, modelName)
    }
    return details, err
}

You can use that to make sure the model exists. You'll still need to get the qualified model name, which you can get like this:

if !jujuclient.IsQualifiedModelName(modelName) {
    store := modelcmd.QualifyingClientStore{c.ClientStore()}
    var err error
    modelName, err = store.QualifiedModelName(c.ControllerName(), modelName)
    if err != nil {...}
}

(you can do it by joining the owner name and model name if you really think that's best, but I'd rather the logic were centralised)

@wallyworld

wallyworld Aug 3, 2017

Owner

I've refactored the code and also fixed a few other places like model-show and destroy-model to make those use the shared code as well.

axw approved these changes Aug 3, 2017

thank you

Owner

wallyworld commented Aug 3, 2017

$$merge$$

Contributor

jujubot commented Aug 3, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot jujubot merged commit b9e7696 into juju:develop Aug 3, 2017

1 check passed

github-check-merge-juju Use !!.*!! to request another build. IE, !!build!!, !!retry!!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment