LGTM +1

On Thu, Jan 26, 2012 at 11:50 AM, Kapil Thangavelu
<email address hidden> wrote:
> Reviewers: mp+90321_code.launchpad.net,
>
> Message:
> Please take a look.
>
> Description:
> It was previously possible to deploy a service with an invalid config,
> which
> would result in a service with no units active in the environment. With
> the
> change on invalid config with deploy, no service will be created in the
> environment.
>
> https://code.launchpad.net/~hazmat/juju/deploy-invalid-conf/+merge/90321
>
> (do not edit description out of merge proposal)
>
>
> Please review this at https://codereview.appspot.com/5572068/
>
> Affected files:
>   M juju/control/deploy.py
>   M juju/control/tests/test_deploy.py
>
>
> Index: juju/control/deploy.py
> === <email address hidden> >
> <email address hidden>
> === modified file 'juju/control/deploy.py'
> --- juju/control/deploy.py      2011-12-07 02:26:56 +0000
> +++ juju/control/deploy.py      2012-01-26 19:39:06 +0000
> @@ -65,7 +65,7 @@
>          num_units=options.num_units)
>
>
> -def parse_config_options(config_file, service_name):
> +def parse_config_options(config_file, service_name, charm):
>      if not os.path.exists(config_file) or \
>              not os.access(config_file, os.R_OK):
>          raise ServiceConfigValueError(
> @@ -77,8 +77,9 @@
>          raise ServiceConfigValueError(
>              "Invalid options file passed to --config.\n"
>              "Expected a YAML dict with service name (%r)." % service_name)
> -    # remove the service name prefix for application to state
> -    return options[service_name]
> +
> +    # Validate and type service options and return
> +    return charm.config.validate(options[service_name])
>
>
>  @inlineCallbacks
> @@ -93,14 +94,15 @@
>      repo, charm_url = resolve(
>          charm_name, repository_path, environment.default_series)
>
> +    charm = yield repo.find(charm_url)
> +    charm_id = str(charm_url.with_revision(charm.get_revision()))
> +
>      # Validate config options prior to deployment attempt
>      service_options = {}
>      service_name = service_name or charm_url.name
>      if config_file:
> -        service_options = parse_config_options(config_file, service_name)
> -
> -    charm = yield repo.find(charm_url)
> -    charm_id = str(charm_url.with_revision(charm.get_revision()))
> +        service_options = parse_config_options(
> +            config_file, service_name, charm)
>
>      provider = environment.get_machine_provider()
>      placement_policy = provider.get_placement_policy()
>
>
> Index: juju/control/tests/test_deploy.py
> === <email address hidden> >
> <email address hidden>
> === modified file 'juju/control/tests/test_deploy.py'
> --- juju/control/tests/test_deploy.py   2011-11-29 20:49:04 +0000
> +++ juju/control/tests/test_deploy.py   2012-01-26 19:39:06 +0000
> @@ -8,7 +8,7 @@
>  from juju.environment.config import EnvironmentsConfig
>  from juju.charm.errors import ServiceConfigValueError
>  from juju.state.environment import EnvironmentStateManager
> -from juju.state.errors import ServiceStateNameInUse
> +from juju.state.errors import ServiceStateNameInUse, ServiceStateNotFound
>  from juju.state.service import ServiceStateManager
>  from juju.state.relation import RelationStateManager
>
> @@ -290,6 +290,24 @@
>              "Expected a YAML dict with service name ('myblog').",
> str(error))
>
>      @inlineCallbacks
> +    def test_deploy_with_invalid_config(self):
> +        """Can't deploy with config that doesn't pass charm validation."""
> +        config_file = self.makeFile(
> +            yaml.dump(dict(myblog=dict(application_file="foo"))))
> +        environment = self.config.get("firstenv")
> +
> +        failure = deploy.deploy(
> +            self.config, environment,
> self.unbundled_repo_path, "local:sample",
> +            "myblog", logging.getLogger("deploy"), config_file)
> +        error = yield self.assertFailure(failure, ServiceConfigValueError)
> +        self.assertIn(
> +            "application_file is not a valid configuration option",
> +            str(error))
> +        yield self.assertFailure(
> +            ServiceStateManager(self.client).get_service_state("myblog"),
> +            ServiceStateNotFound)
> +
> +    @inlineCallbacks
>      def test_deploy_with_config(self):
>          """Valid config options should be available to the deployed
>          service."""
>
>
>
>
>
> --
> https://code.launchpad.net/~hazmat/juju/deploy-invalid-conf/+merge/90321
> You are subscribed to branch lp:juju.