Merge lp:~hazmat/pyjuju/deploy-invalid-conf into lp:pyjuju

[Kapil Thangavelu]

Validate config options before deploying.

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://codereview.appspot.com/5572068/

[Kapil Thangavelu (hazmat)]

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 RelationSta...

[Benjamin Saller (bcsaller)]

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 Environmen...

[Jim Baker (jimbaker)]

+1, looks good to me. Nearly a trivial.

Looks like Ben should have marked his comment approved too, so I'll will approve the branch too.

review: Approve

=== 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:46:25 +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()
 
=== 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:46:25 +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."""