10:58 a.m.
Hi all,
This revision of the series fixes the lack of tests for the new
functionality, and also fixes the issue where subsystems would not
have been deployed correctly (or validated).
Repo: git://git.baserock.org/baserock/baserock/morph.git
Ref: baserock/adamcoldrick/deploy-specific-systems-v3
SHA1: 10130eae94d567a27d5b34410314f2a68b0fa352
Target: master
Adam Coldrick (6):
Don't validate when loading all morphologies
Validate cluster morphologies
Add unit test to test the validation of cluster morphs
Allow the user to specify deployments in a cluster
Update the help text for morph deploy
Add yarn tests for new deployment functionality
morphlib/morphloader.py | 19 ++++++++++-
morphlib/morphloader_tests.py | 15 +++++++++
morphlib/plugins/deploy_plugin.py | 36 +++++++++++++++------
morphlib/sysbranchdir.py | 4 ++-
yarns/deployment.yarn | 67 +++++++++++++++++++++++++++++++++++++++
yarns/implementations.yarn | 3 ++
6 files changed, 133 insertions(+), 11 deletions(-)
--
1.9.1
10:58 a.m.
New subject: [PATCH 1/6] Don't validate when loading all morphologies
This validation happened due to confusion regarding using morph to
checkout baserock:baserock/morph rather than baserock:baserock/morphs.
Since we now use baserock:baserock/definitions, this confusion will no
longer occur.
---
morphlib/sysbranchdir.py | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/morphlib/sysbranchdir.py b/morphlib/sysbranchdir.py
index 9d96e97..23b54eb 100644
--- a/morphlib/sysbranchdir.py
+++ b/morphlib/sysbranchdir.py
@@ -178,7 +178,9 @@ class SystemBranchDirectory(object):
mf = morphlib.morphologyfinder.MorphologyFinder(gd)
for morph in mf.list_morphologies():
text, filename = mf.read_morphology(morph)
- m = loader.load_from_string(text, filename=filename)
+ m = loader.parse_morphology_text(text, filename)
+ loader.set_defaults(m)
+ m.filename = filename
m.repo_url = self.root_repository_url
m.ref = self.system_branch_name
yield m
--
1.9.1
12:24 p.m.
New subject: [PATCH 1/6] Don't validate when loading all morphologies
On Tue, May 27, 2014 at 09:58:36AM +0100, Adam Coldrick wrote:
This validation happened due to confusion regarding using morph to
checkout baserock:baserock/morph rather than baserock:baserock/morphs.
Since we now use baserock:baserock/definitions, this confusion will no
longer occur.
I'm a little worried that this change will mean that there's important
code paths that will not be validated, and instead get an unhelpful
stack trace.
Currently the only place the MorphologyLoader validates morphologies is
in the load_from_string call you are deliberately avoiding here.
6 p.m.
New subject: [PATCH 1/6] Don't validate when loading all morphologies
On Tue, 2014-05-27 at 11:24 +0100, Richard Maw wrote:
On Tue, May 27, 2014 at 09:58:36AM +0100, Adam Coldrick wrote:
> This validation happened due to confusion regarding using morph to
> checkout baserock:baserock/morph rather than baserock:baserock/morphs.
> Since we now use baserock:baserock/definitions, this confusion will no
> longer occur.
I'm a little worried that this change will mean that there's important
code paths that will not be validated, and instead get an unhelpful
stack trace.
Currently the only place the MorphologyLoader validates morphologies is
in the load_from_string call you are deliberately avoiding here.
In that case, we should validate things somewhere else, although I am
not sure where. However, we definitely shouldn't be validating every
morphology in the definitions repository every time we run a command
that creates a build branch. Currently, morph edit validates all
morphologies too (rather than just the ones related to that which is
being edited).
This validation leads to annoying situations like completely unrelated
bad cluster morphologies meaning you can't deploy a cluster morph you
just wrote.
For example, the releases.morph cluster morph in definitions currently
does not match the validation criteria that this patch enforces (unique
deployment names). This means that I now cannot use morph build|deploy|
edit in this checkout without first fixing releases.morph (and anything
else which may be there with invalid syntax).
This patch stops that problem by not validating when we load all
morphologies. However, as you say this leads to morphologies not being
validated when building, which is not good. the load_all_morphologies()
function is only called when creating a MorphologySet, in order to
traverse the morphs and change things in them.
It would make sense to me logically for only affected morphologies to be
validated, as the unaffected morphologies don't actually matter.
However, it could be the case that there is a morphology which could
cause an error in the traversal due to bad syntax/whatever (for example,
we assume the morph has a kind field). That makes me think that this
validation is important.
Do you have an opinion on what should be done with respect to this
validation? Should we keep on validating the whole repository every time
we want to build or deploy something?
_______________________________________________
baserock-dev mailing list
baserock-dev(a)baserock.org
http://listmaster.pepperfish.net/cgi-bin/mailman/listinfo/baserock-dev-ba...
6:14 p.m.
New subject: [PATCH 1/6] Don't validate when loading all morphologies
On Mon, Jun 09, 2014 at 05:00:12PM +0100, Adam Coldrick wrote:
Do you have an opinion on what should be done with respect to this
validation?
Morph should not attempt to validate morphologies during checkout or
branch. I was mistaken when I thought this was a good thing to do.
It should validate morphologies that need to be edited or built.
It shouldn't validate morphologies that don't.
Should we keep on validating the whole repository every time
we want to build or deploy something?
Validating the whole repository came about because I wanted to simplify
the loader code. It's much simpler to load and validate all morphologies
than to traverse them. There is code in morph to do this traversal,
but it is not easy to separate out of its current use.
The traversal code is also old, expecting that it needs to check each
repository, since it was written when we were expecting to have strata
in other repositories.
10:58 a.m.
New subject: [PATCH 2/6] Validate cluster morphologies
The names of deployments in cluster morphologies will need to be unique
in order for the deployment of selected systems to work.
---
morphlib/morphloader.py | 19 ++++++++++++++++++-
1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/morphlib/morphloader.py b/morphlib/morphloader.py
index ca5902a..a174123 100644
--- a/morphlib/morphloader.py
+++ b/morphlib/morphloader.py
@@ -205,6 +205,14 @@ class MultipleValidationErrors(morphlib.Error):
self.msg += ('\t' + str(error))
+class DuplicateDeploymentNameError(morphlib.Error):
+
+ def __init__(self, duplicates):
+ morphlib.Error.__init__(
+ self, 'Cluster morphology contains the following non-unique '
+ 'deployment names:\n %s' % '\n'.join(duplicates))
+
+
class OrderedDumper(yaml.SafeDumper):
keyorder = (
'name',
@@ -412,7 +420,16 @@ class MorphologyLoader(object):
getattr(self, '_validate_%s' % kind)(morph)
def _validate_cluster(self, morph):
- pass
+ # Deployment names must be unique within a cluster
+ deployments = collections.Counter()
+ for system in morph['systems']:
+ deployments.update(system['deploy'].iterkeys())
+ for subsystem in system.get('subsystems', []):
+ deployments.update(subsystem['deploy'].iterkeys())
+ duplicates = set(deployment for deployment, count
+ in deployments.iteritems() if count > 1)
+ if duplicates:
+ raise DuplicateDeploymentNameError(duplicates)
def _validate_system(self, morph):
# A system must contain at least one stratum
--
1.9.1
12:26 p.m.
New subject: [PATCH 2/6] Validate cluster morphologies
On Tue, May 27, 2014 at 09:58:37AM +0100, Adam Coldrick wrote:
The names of deployments in cluster morphologies will need to be
unique
in order for the deployment of selected systems to work.
---
morphlib/morphloader.py | 19 ++++++++++++++++++-
1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/morphlib/morphloader.py b/morphlib/morphloader.py
index ca5902a..a174123 100644
--- a/morphlib/morphloader.py
+++ b/morphlib/morphloader.py
@@ -412,7 +420,16 @@ class MorphologyLoader(object):
getattr(self, '_validate_%s' % kind)(morph)
def _validate_cluster(self, morph):
- pass
+ # Deployment names must be unique within a cluster
+ deployments = collections.Counter()
+ for system in morph['systems']:
+ deployments.update(system['deploy'].iterkeys())
+ for subsystem in system.get('subsystems', []):
+ deployments.update(subsystem['deploy'].iterkeys())
Subsystems can be nested to an arbitrary depth, you need to either do this
validation recursively, or have a tree walker iterator do this for you.
You could probably tidy up the code in a few other places with an iterator
that does this.
+ duplicates = set(deployment for deployment, count
+ in deployments.iteritems() if count > 1)
+ if duplicates:
+ raise DuplicateDeploymentNameError(duplicates)
def _validate_system(self, morph):
# A system must contain at least one stratum
6:01 p.m.
New subject: [PATCH 2/6] Validate cluster morphologies
On Tue, 2014-05-27 at 11:26 +0100, Richard Maw wrote:
On Tue, May 27, 2014 at 09:58:37AM +0100, Adam Coldrick wrote:
> The names of deployments in cluster morphologies will need to be unique
> in order for the deployment of selected systems to work.
> ---
> morphlib/morphloader.py | 19 ++++++++++++++++++-
> 1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/morphlib/morphloader.py b/morphlib/morphloader.py
> index ca5902a..a174123 100644
> --- a/morphlib/morphloader.py
> +++ b/morphlib/morphloader.py
> @@ -412,7 +420,16 @@ class MorphologyLoader(object):
> getattr(self, '_validate_%s' % kind)(morph)
>
> def _validate_cluster(self, morph):
> - pass
> + # Deployment names must be unique within a cluster
> + deployments = collections.Counter()
> + for system in morph['systems']:
> + deployments.update(system['deploy'].iterkeys())
> + for subsystem in system.get('subsystems', []):
> + deployments.update(subsystem['deploy'].iterkeys())
Subsystems can be nested to an arbitrary depth, you need to either do this
validation recursively, or have a tree walker iterator do this for you.
You could probably tidy up the code in a few other places with an iterator
that does this.
Fixed up. Is there documentation somewhere about features of morphology
files like subsystems?
> + duplicates = set(deployment for deployment, count
> + in deployments.iteritems() if count > 1)
> + if duplicates:
> + raise DuplicateDeploymentNameError(duplicates)
>
> def _validate_system(self, morph):
> # A system must contain at least one stratum
_______________________________________________
baserock-dev mailing list
baserock-dev(a)baserock.org
http://listmaster.pepperfish.net/cgi-bin/mailman/listinfo/baserock-dev-ba...
6:19 p.m.
New subject: [PATCH 2/6] Validate cluster morphologies
On Mon, Jun 09, 2014 at 05:01:28PM +0100, Adam Coldrick wrote:
Fixed up. Is there documentation somewhere about features of
morphology
files like subsystems?
I was lazy, I think the only documentation of intent was the RFC that
went with this, the example cluster morphologies and the yarn tests,
rather than it being officially encoded in official documentation sources.
In general though, I think this should go in either the deploy command's
help, or add information about morphologies to the `morph help` command.
10:58 a.m.
New subject: [PATCH 3/6] Add unit test to test the validation of cluster morphs
---
morphlib/morphloader_tests.py | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/morphlib/morphloader_tests.py b/morphlib/morphloader_tests.py
index a050e10..5efcd68 100644
--- a/morphlib/morphloader_tests.py
+++ b/morphlib/morphloader_tests.py
@@ -458,6 +458,21 @@ build-system: dummy
self.loader.validate(m)
self.assertEqual(cm.exception.strata, ["foo"])
+ def test_validate_requires_unique_deployment_names_in_cluster(self):
+ subsystem = [{'morph': 'baz', 'deploy':
{'foobar': None}}]
+ m = morphlib.morph3.Morphology(
+ name='cluster',
+ kind='cluster',
+ systems=[{'morph': 'foo',
+ 'deploy': {'deployment': {}},
+ 'subsystems': subsystem},
+ {'morph': 'bar',
+ 'deploy': {'deployment': {}},
+ 'subsystems': subsystem}])
+ self.assertRaises(
+ morphlib.morphloader.DuplicateDeploymentNameError,
+ self.loader.validate, m)
+
def test_loads_yaml_from_string(self):
string = '''\
name: foo
--
1.9.1
10:58 a.m.
New subject: [PATCH 4/6] Allow the user to specify deployments in a cluster
Instead of taking the name of a cluster morphology and zero or more
parameters for overriding the cluster morphology, morph deploy should
take the name of a cluster morphology and the names of zero or more
system deployments that are defined in the cluster morphology. If no
deployment names are given then all deployments are deployed.
The parameters can still be passed in the same format as before using
the `--param="SYSTEM.KEY=VALUE"` option.
---
morphlib/plugins/deploy_plugin.py | 30 ++++++++++++++++++++++--------
1 file changed, 22 insertions(+), 8 deletions(-)
diff --git a/morphlib/plugins/deploy_plugin.py b/morphlib/plugins/deploy_plugin.py
index 1d58294..378df21 100644
--- a/morphlib/plugins/deploy_plugin.py
+++ b/morphlib/plugins/deploy_plugin.py
@@ -35,10 +35,9 @@ class DeployPlugin(cliapp.Plugin):
'existing cluster of systems rather than do '
'an initial deployment',
group=group_deploy)
-
self.app.add_subcommand(
'deploy', self.deploy,
- arg_synopsis='CLUSTER [SYSTEM.KEY=VALUE]')
+ arg_synopsis='CLUSTER [DEPLOYMENT...] [SYSTEM.KEY=VALUE]')
def disable(self):
pass
@@ -275,7 +274,6 @@ class DeployPlugin(cliapp.Plugin):
'/', 0)
cluster_name = morphlib.util.strip_morph_extension(args[0])
- env_vars = args[1:]
ws = morphlib.workspace.open('.')
sb = morphlib.sysbranchdir.open_from_within('.')
@@ -302,6 +300,17 @@ class DeployPlugin(cliapp.Plugin):
"Error: morph deploy is only supported for cluster"
" morphologies.")
+ # parse the rest of the args
+ all_deployments = set()
+ deployments = set()
+ for system in cluster_morphology['systems']:
+ all_deployments.update([sys_id for sys_id in system['deploy']])
+ for item in args[1:]:
+ if not item in all_deployments:
+ break
+ deployments.add(item)
+ env_vars = args[len(deployments) + 1:]
+
bb = morphlib.buildbranch.BuildBranch(sb, build_ref_prefix,
push_temporary=False)
with contextlib.closing(bb) as bb:
@@ -335,7 +344,7 @@ class DeployPlugin(cliapp.Plugin):
self.deploy_system(build_command, deploy_tempdir,
root_repo_dir, bb.root_repo_url,
bb.root_ref, system, env_vars,
- parent_location='')
+ deployments, parent_location='')
finally:
shutil.rmtree(deploy_tempdir)
@@ -343,7 +352,11 @@ class DeployPlugin(cliapp.Plugin):
def deploy_system(self, build_command, deploy_tempdir,
root_repo_dir, build_repo, ref, system, env_vars,
- parent_location):
+ deployments, parent_location):
+ sys_ids = [sys_id for sys_id, _ in system['deploy'].iteritems()]
+ if deployments and not \
+ any(sys_id in deployments for sys_id in sys_ids):
+ return
old_status_prefix = self.app.status_prefix
system_status_prefix = '%s[%s]' % (old_status_prefix,
system['morph'])
self.app.status_prefix = system_status_prefix
@@ -356,8 +369,9 @@ class DeployPlugin(cliapp.Plugin):
artifact = build_command.resolve_artifacts(srcpool)
deploy_defaults = system.get('deploy-defaults', {})
- deployments = system['deploy']
- for system_id, deploy_params in deployments.iteritems():
+ for system_id, deploy_params in system['deploy'].iteritems():
+ if not system_id in deployments and deployments:
+ continue
deployment_status_prefix = '%s[%s]' % (
system_status_prefix, system_id)
self.app.status_prefix = deployment_status_prefix
@@ -398,7 +412,7 @@ class DeployPlugin(cliapp.Plugin):
for subsystem in system.get('subsystems', []):
self.deploy_system(build_command, deploy_tempdir,
root_repo_dir, build_repo,
- ref, subsystem, env_vars,
+ ref, subsystem, env_vars, [],
parent_location=system_tree)
if parent_location:
deploy_location = os.path.join(parent_location,
--
1.9.1
12:45 p.m.
New subject: [PATCH 4/6] Allow the user to specify deployments in a cluster
On Tue, May 27, 2014 at 09:58:39AM +0100, Adam Coldrick wrote:
Instead of taking the name of a cluster morphology and zero or more
parameters for overriding the cluster morphology, morph deploy should
take the name of a cluster morphology and the names of zero or more
system deployments that are defined in the cluster morphology. If no
deployment names are given then all deployments are deployed.
The parameters can still be passed in the same format as before using
the `--param="SYSTEM.KEY=VALUE"` option.
This part of the commit message is left over from the previous version.
---
morphlib/plugins/deploy_plugin.py | 30 ++++++++++++++++++++++--------
1 file changed, 22 insertions(+), 8 deletions(-)
diff --git a/morphlib/plugins/deploy_plugin.py b/morphlib/plugins/deploy_plugin.py
index 1d58294..378df21 100644
--- a/morphlib/plugins/deploy_plugin.py
+++ b/morphlib/plugins/deploy_plugin.py
@@ -35,10 +35,9 @@ class DeployPlugin(cliapp.Plugin):
'existing cluster of systems rather than do '
'an initial deployment',
group=group_deploy)
-
self.app.add_subcommand(
'deploy', self.deploy,
- arg_synopsis='CLUSTER [SYSTEM.KEY=VALUE]')
+ arg_synopsis='CLUSTER [DEPLOYMENT...] [SYSTEM.KEY=VALUE]')
def disable(self):
pass
@@ -275,7 +274,6 @@ class DeployPlugin(cliapp.Plugin):
'/', 0)
cluster_name = morphlib.util.strip_morph_extension(args[0])
- env_vars = args[1:]
ws = morphlib.workspace.open('.')
sb = morphlib.sysbranchdir.open_from_within('.')
@@ -302,6 +300,17 @@ class DeployPlugin(cliapp.Plugin):
"Error: morph deploy is only supported for cluster"
" morphologies.")
+ # parse the rest of the args
+ all_deployments = set()
+ deployments = set()
+ for system in cluster_morphology['systems']:
+ all_deployments.update([sys_id for sys_id in system['deploy']])
You can use a generator expression here instead of a list comprehension
by omitting the outer [].
+ for item in args[1:]:
+ if not item in all_deployments:
+ break
+ deployments.add(item)
+ env_vars = args[len(deployments) + 1:]
I'm not suggesting you should use this, but the following does the same
thing in a more functional manner. It may be of interest to you.
res = dict(itertools.groupby(args[1:], x.__contains__))
deployments = set(res[True])
env_vars = set(res[False])
@@ -343,7 +352,11 @@ class DeployPlugin(cliapp.Plugin):
def deploy_system(self, build_command, deploy_tempdir,
root_repo_dir, build_repo, ref, system, env_vars,
- parent_location):
+ deployments, parent_location):
+ sys_ids = [sys_id for sys_id, _ in system['deploy'].iteritems()]
You don't use sys_ids for anything but the test here, so I'd be tempted
to create sys_ids as a set instead, as the lookup is cheaper after you
get past trivial sizes.
+ if deployments and not \
+ any(sys_id in deployments for sys_id in sys_ids):
+ return
old_status_prefix = self.app.status_prefix
system_status_prefix = '%s[%s]' % (old_status_prefix,
system['morph'])
self.app.status_prefix = system_status_prefix
6:05 p.m.
New subject: [PATCH 4/6] Allow the user to specify deployments in a cluster
On Tue, 2014-05-27 at 11:45 +0100, Richard Maw wrote:
On Tue, May 27, 2014 at 09:58:39AM +0100, Adam Coldrick wrote:
> Instead of taking the name of a cluster morphology and zero or more
> parameters for overriding the cluster morphology, morph deploy should
> take the name of a cluster morphology and the names of zero or more
> system deployments that are defined in the cluster morphology. If no
> deployment names are given then all deployments are deployed.
>
> The parameters can still be passed in the same format as before using
> the `--param="SYSTEM.KEY=VALUE"` option.
This part of the commit message is left over from the previous version.
Oops, I'll fix that, thanks.
> ---
> morphlib/plugins/deploy_plugin.py | 30 ++++++++++++++++++++++--------
> 1 file changed, 22 insertions(+), 8 deletions(-)
>
> diff --git a/morphlib/plugins/deploy_plugin.py b/morphlib/plugins/deploy_plugin.py
> index 1d58294..378df21 100644
> --- a/morphlib/plugins/deploy_plugin.py
> +++ b/morphlib/plugins/deploy_plugin.py
> @@ -35,10 +35,9 @@ class DeployPlugin(cliapp.Plugin):
> 'existing cluster of systems rather than do
'
> 'an initial deployment',
> group=group_deploy)
> -
> self.app.add_subcommand(
> 'deploy', self.deploy,
> - arg_synopsis='CLUSTER [SYSTEM.KEY=VALUE]')
> + arg_synopsis='CLUSTER [DEPLOYMENT...] [SYSTEM.KEY=VALUE]')
>
> def disable(self):
> pass
> @@ -275,7 +274,6 @@ class DeployPlugin(cliapp.Plugin):
> '/', 0)
>
> cluster_name = morphlib.util.strip_morph_extension(args[0])
> - env_vars = args[1:]
>
> ws = morphlib.workspace.open('.')
> sb = morphlib.sysbranchdir.open_from_within('.')
> @@ -302,6 +300,17 @@ class DeployPlugin(cliapp.Plugin):
> "Error: morph deploy is only supported for cluster"
> " morphologies.")
>
> + # parse the rest of the args
> + all_deployments = set()
> + deployments = set()
> + for system in cluster_morphology['systems']:
> + all_deployments.update([sys_id for sys_id in
system['deploy']])
You can use a generator expression here instead of a list comprehension
by omitting the outer [].
Changed, thanks.
> + for item in args[1:]:
> + if not item in all_deployments:
> + break
> + deployments.add(item)
> + env_vars = args[len(deployments) + 1:]
I'm not suggesting you should use this, but the following does the same
thing in a more functional manner. It may be of interest to you.
res = dict(itertools.groupby(args[1:], x.__contains__))
deployments = set(res[True])
env_vars = set(res[False])
That is an interesting snippet, thanks. I quite like that approach.
> @@ -343,7 +352,11 @@ class DeployPlugin(cliapp.Plugin):
>
> def deploy_system(self, build_command, deploy_tempdir,
> root_repo_dir, build_repo, ref, system, env_vars,
> - parent_location):
> + deployments, parent_location):
> + sys_ids = [sys_id for sys_id, _ in system['deploy'].iteritems()]
You don't use sys_ids for anything but the test here, so I'd be tempted
to create sys_ids as a set instead, as the lookup is cheaper after you
get past trivial sizes.
Changed :) .
> + if deployments and not \
> + any(sys_id in deployments for sys_id in sys_ids):
> + return
> old_status_prefix = self.app.status_prefix
> system_status_prefix = '%s[%s]' % (old_status_prefix,
system['morph'])
> self.app.status_prefix = system_status_prefix
_______________________________________________
baserock-dev mailing list
baserock-dev(a)baserock.org
http://listmaster.pepperfish.net/cgi-bin/mailman/listinfo/baserock-dev-ba...
6:11 p.m.
New subject: [PATCH 4/6] Allow the user to specify deployments in a cluster
On Mon, Jun 09, 2014 at 05:05:40PM +0100, Adam Coldrick wrote:
On Tue, 2014-05-27 at 11:45 +0100, Richard Maw wrote:
> > + for item in args[1:]:
> > + if not item in all_deployments:
> > + break
> > + deployments.add(item)
> > + env_vars = args[len(deployments) + 1:]
>
> I'm not suggesting you should use this, but the following does the same
> thing in a more functional manner. It may be of interest to you.
>
> res = dict(itertools.groupby(args[1:], x.__contains__))
> deployments = set(res[True])
> env_vars = set(res[False])
That is an interesting snippet, thanks. I quite like that approach.
I, however, don't like that. :) The for loop seems fairly obvious. The
itertools isn't clear to me even after I know what it's doing: it
requires understanding a lot more things than the for loop, and
figuring out how they work together when used in this way, and that's
a much bigger cognitive burden.
--
http://www.codethink.co.uk/ http://wiki.baserock.org/ http://www.baserock.com/
6:25 p.m.
New subject: [PATCH 4/6] Allow the user to specify deployments in a cluster
On Mon, Jun 09, 2014 at 05:05:40PM +0100, Adam Coldrick wrote:
On Tue, 2014-05-27 at 11:45 +0100, Richard Maw wrote:
> > + for item in args[1:]:
> > + if not item in all_deployments:
> > + break
> > + deployments.add(item)
> > + env_vars = args[len(deployments) + 1:]
>
> I'm not suggesting you should use this, but the following does the same
> thing in a more functional manner. It may be of interest to you.
>
> res = dict(itertools.groupby(args[1:], x.__contains__))
> deployments = set(res[True])
> env_vars = set(res[False])
That is an interesting snippet, thanks. I quite like that approach.
It's not exactly the same behaviour, as itertools.groupby produces a
sequence of (matched, [matched_values]). Passing that to `dict` creates
a dict of the last matches, where we want the first true matches and
assume the rest are env_vars.
You'd have to do further processing and get it to do this, probably
using groupby again.
I just thought it would be interesting to see how you would try to solve
it with the more functional programming-y functions.
10:58 a.m.
New subject: [PATCH 5/6] Update the help text for morph deploy
---
morphlib/plugins/deploy_plugin.py | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/morphlib/plugins/deploy_plugin.py b/morphlib/plugins/deploy_plugin.py
index 378df21..7c59768 100644
--- a/morphlib/plugins/deploy_plugin.py
+++ b/morphlib/plugins/deploy_plugin.py
@@ -49,6 +49,10 @@ class DeployPlugin(cliapp.Plugin):
* `CLUSTER` is the name of the cluster to deploy.
+ * `DEPLOYMENT...` is the name of zero or more deployments in the
+ morphology to deploy. If none are specified then all deployments
+ in the morphology are deployed.
+
* `SYSTEM.KEY=VALUE` can be used to assign `VALUE` to a parameter
named `KEY` for the system identified by `SYSTEM` in the cluster
morphology (see below). This will override parameters defined
@@ -248,7 +252,7 @@ class DeployPlugin(cliapp.Plugin):
Deployment configuration is stored in the deployed system as
/baserock/deployment.meta. THIS CONTAINS ALL ENVIRONMENT VARIABLES SET
- DURINGR DEPLOYMENT, so make sure you have no sensitive information in
+ DURING DEPLOYMENT, so make sure you have no sensitive information in
your environment that is being leaked. As a special case, any
environment/deployment variable that contains 'PASSWORD' in its name is
stripped out and not stored in the final system.
--
1.9.1
10:58 a.m.
New subject: [PATCH 6/6] Add yarn tests for new deployment functionality
---
yarns/deployment.yarn | 67 ++++++++++++++++++++++++++++++++++++++++++++++
yarns/implementations.yarn | 3 +++
2 files changed, 70 insertions(+)
diff --git a/yarns/deployment.yarn b/yarns/deployment.yarn
index fc21b82..b51645c 100644
--- a/yarns/deployment.yarn
+++ b/yarns/deployment.yarn
@@ -102,3 +102,70 @@ deployed system contains the other. Since the baserock directory is
in
every system, we can check for that.
AND tarball test.tar contains var/lib/sysroots/test-system/baserock
+
+Partial deployments
+===================
+
+Deploy part of a cluster
+------------------------
+
+We may not always want to deploy every system in a cluster, for example
+if we only want to deploy a distbuild controller but we have a cluster
+for a full network.
+
+ SCENARIO partially deploying a cluster morphology
+ GIVEN a workspace
+ AND a git server
+ WHEN the user checks out the system branch called master
+ GIVEN a cluster called test-cluster in system branch master
+ AND a system in cluster test-cluster in branch master called test-system
+ AND system test-system in cluster test-cluster in branch master builds test-system
+ AND system test-system in cluster test-cluster in branch master has deployment type:
tar
+
+We now want to add a second system to the cluster, so we can choose
+which one to deploy.
+
+ GIVEN a system in cluster test-cluster in branch master called second-system
+ AND system second-system in cluster test-cluster in branch master builds
second-system
+ AND system second-system in cluster test-cluster in branch master has deployment
type: tar
+
+ WHEN the user builds the system test-system in branch master
+ AND the user attempts to deploy the cluster test-cluster in branch master with
options test-system test-system.location="$DATADIR/test-system.tar"
second-system.location="$DATADIR/second-system.tar"
+ THEN morph succeeded
+
+Morph succeeding alone does not satisfy the requirements of this test.
+It may have deployed both systems, or the wrong system, so we need to
+make sure only the correct tarball exists.
+
+ THEN file test-system.tar exists
+ AND file second-system.tar does not exist
+
+Deploy all of a multi-system cluster
+------------------------------------
+
+We should also test that not specifying a system deploys both systems.
+
+ SCENARIO deploying a cluster morphology that contains multiple systems
+ GIVEN a workspace
+ AND a git server
+ WHEN the user checks out the system branch called master
+ GIVEN a cluster called test-cluster in system branch master
+ AND a system in cluster test-cluster in branch master called test-system
+ AND system test-system in cluster test-cluster in branch master builds test-system
+ AND system test-system in cluster test-cluster in branch master has deployment type:
tar
+
+We now want to add a second system deployment to the cluster, so we can
+choose which one to deploy.
+
+ GIVEN a system in cluster test-cluster in branch master called second-system
+ AND system second-system in cluster test-cluster in branch master builds test-system
+ AND system second-system in cluster test-cluster in branch master has deployment
type: tar
+
+ WHEN the user builds the system test-system in branch master
+ AND the user attempts to deploy the cluster test-cluster in branch master with
options test-system.location="$DATADIR/test-system.tar"
second-system.location="$DATADIR/second-system.tar"
+ THEN morph succeeded
+
+We should also check that both systems were deployed as expected.
+
+ THEN file test-system.tar exists
+ AND file second-system.tar exists
diff --git a/yarns/implementations.yarn b/yarns/implementations.yarn
index e4f3639..7d52a06 100644
--- a/yarns/implementations.yarn
+++ b/yarns/implementations.yarn
@@ -720,6 +720,9 @@ Check attributes of a file on the filesystem
IMPLEMENTS THEN file (\S+) exists
test -e "$DATADIR/$MATCH_1"
+ IMPLEMENTS THEN file (\S+) does not exist
+ test ! -e "$DATADIR/$MATCH_1"
+
IMPLEMENTS THEN file (\S+) has permissions (\S+)
stat -c %A "$DATADIR/$MATCH_1" | grep -Fx -e "$MATCH_2"
--
1.9.1
1:07 p.m.
New subject: [PATCH 6/6] Add yarn tests for new deployment functionality
I don't see any tests for selective deployments with nested deployments.
I expect there to be some trickiness involved there, so they really
should be tested.
On Tue, May 27, 2014 at 09:58:41AM +0100, Adam Coldrick wrote:
---
yarns/deployment.yarn | 67 ++++++++++++++++++++++++++++++++++++++++++++++
yarns/implementations.yarn | 3 +++
2 files changed, 70 insertions(+)
diff --git a/yarns/deployment.yarn b/yarns/deployment.yarn
index fc21b82..b51645c 100644
--- a/yarns/deployment.yarn
+++ b/yarns/deployment.yarn
@@ -102,3 +102,70 @@ deployed system contains the other. Since the baserock directory is
in
<snip>
+ WHEN the user builds the system test-system in branch master
+ AND the user attempts to deploy the cluster test-cluster in branch master with
options test-system test-system.location="$DATADIR/test-system.tar"
second-system.location="$DATADIR/second-system.tar"
I don't like this way of deploying, I'd prefer a sequence of commands
to add the deployment options to the cluster, and a new IMPLEMENTS that
handles being given a list of systems to deploy.
I regret creating this IMPLEMENTS, since it should only be used for
testing deployment variables on the command line. In general we should
add the options to the morphology.
<snip>
6:08 p.m.
New subject: [PATCH 6/6] Add yarn tests for new deployment functionality
On Tue, 2014-05-27 at 12:07 +0100, Richard Maw wrote:
I don't see any tests for selective deployments with nested
deployments.
I expect there to be some trickiness involved there, so they really
should be tested.
I'll add a test for that.
On Tue, May 27, 2014 at 09:58:41AM +0100, Adam Coldrick wrote:
> ---
> yarns/deployment.yarn | 67 ++++++++++++++++++++++++++++++++++++++++++++++
> yarns/implementations.yarn | 3 +++
> 2 files changed, 70 insertions(+)
>
> diff --git a/yarns/deployment.yarn b/yarns/deployment.yarn
> index fc21b82..b51645c 100644
> --- a/yarns/deployment.yarn
> +++ b/yarns/deployment.yarn
> @@ -102,3 +102,70 @@ deployed system contains the other. Since the baserock
directory is in
<snip>
> + WHEN the user builds the system test-system in branch master
> + AND the user attempts to deploy the cluster test-cluster in branch master with
options test-system test-system.location="$DATADIR/test-system.tar"
second-system.location="$DATADIR/second-system.tar"
I don't like this way of deploying, I'd prefer a sequence of commands
to add the deployment options to the cluster, and a new IMPLEMENTS that
handles being given a list of systems to deploy.
I regret creating this IMPLEMENTS, since it should only be used for
testing deployment variables on the command line. In general we should
add the options to the morphology.
That would be a lot nicer, I'll do that.
_______________________________________________
baserock-dev mailing list
baserock-dev(a)baserock.org
http://listmaster.pepperfish.net/cgi-bin/mailman/listinfo/baserock-dev-ba...
12:27 p.m.
New subject: [PATCH 0/6 v2] Allow morph deploy to deploy individual systems from a cluster
On Tue, May 27, 2014 at 09:58:35AM +0100, Adam Coldrick wrote:
Hi all,
This revision of the series fixes the lack of tests for the new
functionality, and also fixes the issue where subsystems would not
have been deployed correctly (or validated).
Repo: git://git.baserock.org/baserock/baserock/morph.git
Ref: baserock/adamcoldrick/deploy-specific-systems-v3
SHA1: 10130eae94d567a27d5b34410314f2a68b0fa352
Target: master
This looks OK to me, +1.
--
http://www.codethink.co.uk/ http://wiki.baserock.org/ http://www.baserock.com/
1:10 p.m.
New subject: [PATCH 0/6 v2] Allow morph deploy to deploy individual systems from a cluster
On Tue, May 27, 2014 at 09:58:35AM +0100, Adam Coldrick wrote:
Hi all,
This revision of the series fixes the lack of tests for the new
functionality, and also fixes the issue where subsystems would not
have been deployed correctly (or validated).
Repo: git://git.baserock.org/baserock/baserock/morph.git
Ref: baserock/adamcoldrick/deploy-specific-systems-v3
SHA1: 10130eae94d567a27d5b34410314f2a68b0fa352
Target: master
There's an incorrect commit message. I would have been ok with this
being fixed up without another patch series.
I'm worried about removing the validation from the system branch loading,
as I'm not sure validation is handled well enough elsewhere.
I would have been happy with an explanation about how this works.
However, nested deployments can be arbitrarily deep, and I don't see
any tests with both nested deployments and selective deployments, and
I suspect this could have weird interactions, so I'm voting -1 on this.
3469
days inactive
3482
days old
20 comments
3 participants
participants (3)
-
Adam Coldrick -
Lars Wirzenius -
Richard Maw