Changes --reset to be a stringvar #6399

Merged
merged 2 commits into from Oct 11, 2016

Conversation

Projects
None yet
3 participants
Contributor

reedobrien commented Oct 7, 2016

Also allows reset and set to run in the same command, per the spec.
All tests pass at this point but it needs more tests

Refs: config command collapse spec https://goo.gl/yqrrPI
Refs: model-config-tree

QA:

With an lxd cloud like (see Faster LXD for details on apt-http proxy and other items, or don't use them)

lxdtest:
    type: lxd
    config:
        apt-http-proxy: http://10.0.4.1:8000
        bootstrap-timeout: 300
        default-series: xenial
        enable-os-refresh-update: true
        enable-os-upgrade: false
        logging-config: <root>=DEBUG
        no-proxy: https://lxd-no-regions
  1. juju bootstrap lxd-model-config lxdtest

  2. juju model-config and verify no-proxy value

    ATTRIBUTE FROM VALUE
    agent-metadata-url default ""
    agent-stream default released
    agent-version model 2.0.0.1
    apt-ftp-proxy default ""
    apt-http-proxy controller http://10.0.4.1:8000
    apt-https-proxy default ""
    apt-mirror default ""
    automatically-retry-hooks default true
    default-series controller xenial
    development default false
    disable-network-management default false
    enable-os-refresh-update controller true
    enable-os-upgrade controller false
    firewall-mode default instance
    ftp-proxy default ""
    http-proxy default ""
    https-proxy default ""
    ignore-machine-addresses default false
    image-metadata-url default ""
    image-stream default released
    logforward-enabled default false
    logging-config model =DEBUG;unit=DEBUG
    no-proxy controller https://lxd-no-regions
    provisioner-harvest-mode default destroyed
    proxy-ssh default false
    resource-tags model {}
    ssl-hostname-verification default true
    test-mode default false
    transmit-vendor-metrics default true

  3. set no-proxy juju model-config no-proxy=different

  4. verify juju model-config no-proxy is different

  5. set others and reset no proxy juju model-config agent-stream=devel apt-mirror=mirror

  6. verify the keys are set for k in apt-mirror agent-stream; do juju model-config $k; done

  7. set these to be different and reset no-proxy juju model-config agent-stream=stage --reset no-proxy apt-mirror=anothermirror

  8. verify for k in apt-mirror agent-stream no-proxy; do juju model-config $k; done

    anothermirror
    stage
    https://lxd-no-regions

  9. reset multiple and set multiple juju model-config no-proxy=foo --reset agent-stream,apt-mirror default-series=yakkety

  10. verify for k in apt-mirror agent-stream no-proxy default-series; do juju model-config $k; done

    ""
    released
    foo
    yakkety

  11. Try invalid command juju model-config no-proxy=foo --reset no-proxy

    error: key "no-proxy" cannot be both set and reset in the same command

  12. Try another juju model-config no-proxy=foo no-proxy=bar

    error: key "no-proxy" specified more than once

  13. Try multiple get juju model-config no-proxy apt-mirror

    error: can only retrieve a single value, or all values

  14. Try reset and get juju model-config --reset no-proxy apt-mirror

    error: cannot set and retrieve model values simultaneously

  15. Try things I didn't think about here

  16. juju kill-controller -y lxd-model-config

Changes --reset to be a stringvar
Also allows reset and set to run in the same command, per the spec.
All tests pass at this point but it needs more tests

Refs: config command collapse spec https://goo.gl/yqrrPI
Refs: model-config-tree

Initial review - looks similar to what's been done for model-defaults so +1 so far.

cmd/juju/model/configcommand.go
+ // make sure that we are not resetting because it is not valid to get and
+ // reset simultaneously.
+ if len(c.reset) > 0 {
+ return errors.New("cannot set and retrieve default values simultaneously")
@wallyworld

wallyworld Oct 7, 2016

Owner

typo s/default/model

@reedobrien

reedobrien Oct 7, 2016

Contributor

derp, fixed.

cmd/juju/model/configcommand_test.go
- args: []string{"--reset", "something", "weird"},
- nilErr: true,
+ args: []string{"--reset", "something", "weird"},
+ errorMatch: "cannot set and retrieve default values simultaneously",
@wallyworld

wallyworld Oct 7, 2016

Owner

s/default/model

@reedobrien

reedobrien Oct 7, 2016

Contributor

derp...

Typo correction and add more tests
Refs: config command collapse spec https://goo.gl/yqrrPI
Refs: model-config-tree
Contributor

reedobrien commented Oct 7, 2016

Also added more test cases. Let me know if you see any more corners.

@reedobrien reedobrien changed the title from WIP: Changes --reset to be a stringvar to Changes --reset to be a stringvar Oct 7, 2016

Contributor

reedobrien commented Oct 11, 2016

$$merge$$

Contributor

jujubot commented Oct 11, 2016

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

@jujubot jujubot merged commit 4159d57 into juju:master Oct 11, 2016

@reedobrien reedobrien deleted the reedobrien:feature/model-config-reset-as-string branch Oct 11, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment