Merge into trunk : deploy-invalid-conf : Code : pyjuju

Status:

Merged

Approved by:

Jim Baker on 2012-02-03

Approved revision:

448

Merged at revision:

455

Proposed branch:

lp://staging/~hazmat/pyjuju/deploy-invalid-conf

Merge into:

lp://staging/pyjuju

Diff against target:

82 lines (+28/-8)

2 files modified

juju/control/deploy.py (+9/-7)
juju/control/tests/test_deploy.py (+19/-1)

To merge this branch:

bzr merge lp://staging/~hazmat/pyjuju/deploy-invalid-conf

Undecided

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Jim Baker (community)		2012-01-26	Approve on 2012-02-03
Review via email: mp+90321@code.staging.launchpad.net

Description of the change

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/

Revision history for this message

Kapil Thangavelu (hazmat) wrote on 2012-01-26:

#

Download full text (4.2 KiB)

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

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
=== kapil.thangavelu@canonical.com-20120121234209-fcuzmsjcd6p9qlil >  
kapil.thangavelu@canonical.com-20120126193906-9q17tye8tyqtgwgr
=== 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
=== kapil.thangavelu@canonical.com-20120121234209-fcuzmsjcd6p9qlil >  
kapil.thangavelu@canonical.com-20120126193906-9q17tye8tyqtgwgr
=== 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."""

Revision history for this message

Benjamin Saller (bcsaller) wrote on 2012-01-26:

#

Download full text (4.6 KiB)

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

LGTM +1

On Thu, Jan 26, 2012 at 11:50 AM, Kapil Thangavelu
<kapil.thangavelu@canonical.com> 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
> === kapil.thangavelu@canonical.com-20120121234209-fcuzmsjcd6p9qlil >
> kapil.thangavelu@canonical.com-20120126193906-9q17tye8tyqtgwgr
> === 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
> === kapil.thangavelu@canonical.com-20120121234209-fcuzmsjcd6p9qlil >
> kapil.thangavelu@canonical.com-20120126193906-9q17tye8tyqtgwgr
> === 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.

Revision history for this message

Jim Baker (jimbaker) wrote on 2012-02-03:

#

+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

pyjuju

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

Commit message

Description of the change

Preview Diff

Subscribers