Merge ~rjschwei/cloud-init:addZyppRepos into cloud-init:master

[Robert Schweikert]

suse: Support addition of zypp repos.

This adds support to cloud-config for adding zypp repos.

Support addition of zypp repos

[Scott Moser (smoser)]

This seems generally fine. And we can take it, but i'd like to do so after the 17.1 release.
I have a few requests:

a.) please add schema (see runcmd as an example). Chad has been workign on adding schema in places. It helps docs and general cloud-init usage.
b.) please do less in handle. handle has an annoying interface. and its easier and more direct to unit test if you break portions out.
c.) don't use the 'log' that is passed into handle, instead import and use LOG.<verb> like other modules do. Your usage makes sense, as you copied other modules, we're just moving away from that.

I dont' want to sound rude, thanks for your contribution, and we will get this in, just want to do things in the way we're moving to rather than like the older code that we're fixing up as we go.

review: Needs Fixing

[Scott Moser (smoser)]

oh, and you'll want to enable the config module in config/cloud.cfg.tmpl.

[Scott Moser (smoser)]

Oh, and one other thing:
 zypp_repos

Can we make the top level key just 'zypp' (or zypper?) i dont know which is correct, but we're really trying to get to fewer top level keys rather.
  zypp:
    repos:
      stuff...here..

rather than
  zypp_repos:
    stuff_here
  zypp_config:
    stuff here....

[Robert Schweikert (rjschwei)]

> This seems generally fine. And we can take it, but i'd like to do so after the
> 17.1 release.
> I have a few requests:
>
> a.) please add schema (see runcmd as an example). Chad has been workign on
> adding schema in places. It helps docs and general cloud-init usage.

OK, having some trouble to figure out the info needed, what I am missing is the details for

'properties': {....}

> I dont' want to sound rude, thanks for your contribution, and we will get this
> in, just want to do things in the way we're moving to rather than like the
> older code that we're fixing up as we go.

Not rude, thanks for looking at this so quickly especially while you are trying to get out a release. Agreed new code should conform to where everything is going, not to where things have been.

[Robert Schweikert (rjschwei)]

> oh, and you'll want to enable the config module in config/cloud.cfg.tmpl.

Yes, now that the tmpl stuff is merged I will rebase and add that. I didn't want to create merg conflict for myself ;)

[Robert Schweikert (rjschwei)]

> Oh, and one other thing:
> zypp_repos
>
> Can we make the top level key just 'zypp' (or zypper?) i dont know which is
> correct, but we're really trying to get to fewer top level keys rather.
> zypp:
> repos:
> stuff...here..
>
> rather than
> zypp_repos:
> stuff_here
> zypp_config:
> stuff here....

zypper would be correct but I don't think I get what you are asking.

[Robert Schweikert (rjschwei)]

> > Oh, and one other thing:
> > zypp_repos
> >
> > Can we make the top level key just 'zypp' (or zypper?) i dont know which is
> > correct, but we're really trying to get to fewer top level keys rather.
> > zypp:
> > repos:
> > stuff...here..
> >
> > rather than
> > zypp_repos:
> > stuff_here
> > zypp_config:
> > stuff here....
>
> zypper would be correct but I don't think I get what you are asking.

Never mind, got it now, duh

c82c116... by Robert Schweikert on 2017-09-21

- Zypper repo handling
  + Rename config option from zypp_add_repo to zypper_add_repo
  + Include setting the configuration
  + Provide schema data for validation
  + Change top level configuration setting to zypper

ab3a0e9... by Robert Schweikert on 2017-09-21

- Run zypper repo setup by default on SUSE distros

[Robert Schweikert (rjschwei)]

@smoser

Can you take another look once the release is out the door. Still need to re-visit the test but I wanted to make sure the code is going in the right direction before I start implementing the test.

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:ab3a0e9057319580b73fcbd4ac89d8d9277bda35
https://jenkins.ubuntu.com/server/job/cloud-init-ci/346/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/346/rebuild

review: Needs Fixing (continuous-integration)

3476d6a... by Robert Schweikert on 2017-09-22

- Add SUSE repos
  + Update repo description and configuration format
  + Unit testing
  + Complete schema definition

[Robert Schweikert (rjschwei)]

Ready for another review.

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:3476d6ab76922accc0c8ca9466f935e0aed609e3
https://jenkins.ubuntu.com/server/job/cloud-init-ci/350/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/350/rebuild

review: Needs Fixing (continuous-integration)

[Scott Moser (smoser)]

just one comment in l ine. and note the bot didn't like something.

i'll review further a bit later.

d93870a... by Robert Schweikert on 2017-09-24

- Improve example doc for zypper_add_repo
- Change id to cc_zypper_add_repo to match naming convention
- Add new schema definition to schema test

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:d93870abb9668863c91ebd897ac6fd56ebc219c4
https://jenkins.ubuntu.com/server/job/cloud-init-ci/351/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/351/rebuild

review: Needs Fixing (continuous-integration)

4de284c... by Robert Schweikert on 2017-09-26

- Style fixes

[Robert Schweikert (rjschwei)]

@smoser OK, one more try, not sure why the style slipped through make style-check. Anyway ready for your review. Any chance you can get to this in the next couple of days? Thanks

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:4de284ca40c57e1f6a4a09f0136a856f19c94c6c
https://jenkins.ubuntu.com/server/job/cloud-init-ci/356/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/356/rebuild

review: Needs Fixing (continuous-integration)

[Robert Schweikert (rjschwei)]

I do not get the test failure, meaning why it shows up in the bot.

Locally:

> flake8 --version
3.3.0 (mccabe: 0.6.1, naming: 0.4.1, pycodestyle: 2.3.1, pyflakes: 1.5.0) CPython 2.7.13 on Linux
> flake8 cloudinit/config/cc_zypper_add_repo.py

returns clean on my machine. My a difference between py3 flake 8 and py 2 flake8?

b550f2d... by Robert Schweikert on 2017-09-29

- fix alpha order for importing

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:b550f2dfd944d89e967c9d72829fba0748575ac3
https://jenkins.ubuntu.com/server/job/cloud-init-ci/362/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/362/rebuild

review: Needs Fixing (continuous-integration)

e4ed9ec... by Robert Schweikert on 2017-09-29

- import alpha order part 2

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:e4ed9ec76a82fb1168b8e92b4ea8a98463b0acfa
https://jenkins.ubuntu.com/server/job/cloud-init-ci/363/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/363/rebuild

review: Needs Fixing (continuous-integration)

[Chad Smith (chad.smith)]

Thanks for these branches Robert. Here's a minor set of patches with some of the comments I suggested. I hadn't gotten over to the unit tests yet, but I'm hoping to get that for you tomorrow morning.

Some changes suggested.
http://paste.ubuntu.com/25658611/

[Robert Schweikert (rjschwei)]

If http://paste.ubuntu.com/25658611/ is what it takes to get this merged I'll make the changes, however, I do not like the idea of having

_write_repos call _write_repository_configs

and having _write_repository_configs be responsible for multiple things as indicated in the doc string by the "and". IMHO, any time an implementation has an "and" as part of it's documentation that smells like side effect programming which IMHO is not a good practice.

But as I said, whatever it takes to get it merged and to avoid me carrying a patch in the SUSE package.

a4d7d5a... by Robert Schweikert on 2017-10-02

- more "pythonic"

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:a4d7d5adefe2e411d709069f7cd539014fccb285
https://jenkins.ubuntu.com/server/job/cloud-init-ci/366/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/366/rebuild

review: Needs Fixing (continuous-integration)

[Chad Smith (chad.smith)]

Robert, that's not what it takes to get the approval, but rather a quick set of suggestions/opinions on parts of the structure of the code.

I admit my quick pass at this review needs validation and/or discussion/rejection. Feel free to reject parts as undesireable.

I generally agree on your aversion to the multi-function methods, though there was a tension when I saw the code using configobj but not using configobj.write. It feels like the code was forcing functional separation when the configobj we created can already do this work for us. I don't really know how to resolve that tension, which is why I threw together the hacky patch.

Admitedly, I hadn't looked deeply enough at this branch to get the unit tests passing yet, but there are some things that felt like they rubbed. I had wanted to get you the unrefined pastebin to get the conversation started. I'll spend more time on this this morning when I start my day to see if there is an approach that makes more sense.

[Robert Schweikert (rjschwei)]

I think at this point if we have follow ups for style, refinement lets nail that down in a follow up request. Dito for the tests.

[Chad Smith (chad.smith)]

[1] Couple flakes:

cloudinit/config/cc_zypper_add_repo.py:15:1: H306: imports not in alphabetical order (cloudinit.util, cloudinit.config.schema.get_schema_doc)
tests/unittests/test_handler/test_handler_zypper_add_repo.py:18:1: F401 'tempfile' imported but unused.

[2] Minor tweak on existing unit tests in your setup method, use self.tmp_dir() as you have in other tests and drop the shutil import (and addClennup)

Per any ConfigObj comments, I think we generally have to deal w/ ConfigParser -> ConfigObj moves throughout cloudinit, so we might make any changes related to that at another time.

thanks again for this.

review: Approve

9485f74... by Robert Schweikert on 2017-10-03

- Test improvements for zypper repo add

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:9485f742ee4083d01d9ad7f50842f628176c58db
https://jenkins.ubuntu.com/server/job/cloud-init-ci/373/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/373/rebuild

review: Needs Fixing (continuous-integration)

[Scott Moser (smoser)]

changing my review. Thanks for patience Robert. I'll fix your flake8 error, and then make sure this runs in tox and then accept.

Thanks.

review: Approve

diff --git a/cloudinit/config/cc_zypper_add_repo.py b/cloudinit/config/cc_zypper_add_repo.py
new file mode 100644
index 0000000..2cc5517
--- /dev/null
+++ b/cloudinit/config/cc_zypper_add_repo.py
@@ -0,0 +1,220 @@
+#
+#    Copyright (C) 2017 SUSE LLC.
+#
+# This file is part of cloud-init. See LICENSE file for license information.
+
+"""zypper_add_repo: Add zyper repositories to the system"""
+
+
+import configobj
+import os
+
+
+from cloudinit import log as logging
+from cloudinit import util
+from cloudinit.config.schema import get_schema_doc
+from cloudinit.settings import PER_ALWAYS
+from six import string_types
+from textwrap import dedent
+
+distros = ['opensuse', 'sles']
+
+schema = {
+    'id': 'cc_zypper_add_repo',
+    'name': 'ZypperAddRepo',
+    'title': 'Configure zypper behavior and add zypper repositories',
+    'description': dedent("""\
+        Configure zypper behavior by modifying /etc/zypp/zypp.conf. The
+        configuration writer is "dumb" and will simply append the provided
+        configuration options to the configuration file. Option settings
+        that may be duplicate will be resolved by the way the zypp.conf file
+        is parsed. The file is in INI format.
+        Add repositories to the system. No validation is performed on the
+        repository file entries, it is assumed the user is familiar with
+        the zypper repository file format."""),
+    'distros': distros,
+    'examples': [dedent("""\
+        zypper:
+          repos:
+            - id: opensuse-oss
+              name: os-oss
+              baseurl: http://dl.opensuse.org/dist/leap/v/repo/oss/
+              enabled: 1
+              autorefresh: 1
+            - id: opensuse-oss-update
+              name: os-oss-up
+              baseurl: http://dl.opensuse.org/dist/leap/v/update
+              # any setting per
+              # https://en.opensuse.org/openSUSE:Standards_RepoInfo
+              # enable and autorefresh are on by default
+          config:
+            reposdir: /etc/zypp/repos.dir
+            servicesdir: /etc/zypp/services.d
+            download.use_deltarpm: true
+            # any setting in /etc/zypp/zypp.conf
+    """)],
+    'frequency': PER_ALWAYS,
+    'type': 'object',
+    'properties': {
+        'zypper': {
+            'type': 'object',
+            'properties': {
+                'repos': {
+                    'type': 'array',
+                    'items': {
+                        'type': 'object',
+                        'properties': {
+                            'id': {
+                                'type': 'string',
+                                'description': dedent("""\
+                                    The unique id of the repo, used when
+                                     writing
+                                    /etc/zypp/repos.d/<id>.repo.""")
+                            },
+                            'baseurl': {
+                                'type': 'string',
+                                'format': 'uri',   # built-in format type
+                                'description': 'The base repositoy URL'
+                            }
+                        },
+                        'required': ['id', 'baseurl'],
+                        'additionalProperties': True
+                    },
+                    'minItems': 1
+                },
+                'config': {
+                    'type': 'object',
+                    'description': dedent("""\
+                        Any supported zypo.conf key is written to
+                        /etc/zypp/zypp.conf'""")
+                }
+            },
+            'required': [],
+            'minProperties': 1,  # Either config or repo must be provided
+            'additionalProperties': False,  # only repos and config allowed
+        }
+    }
+}
+
+__doc__ = get_schema_doc(schema)  # Supplement python help()
+
+LOG = logging.getLogger(__name__)
+
+
+def _canonicalize_id(repo_id):
+    repo_id = repo_id.replace(" ", "_")
+    return repo_id
+
+
+def _format_repo_value(val):
+    if isinstance(val, bool):
+        # zypp prefers 1/0
+        return 1 if val else 0
+    if isinstance(val, (list, tuple)):
+        return "\n    ".join([_format_repo_value(v) for v in val])
+    if not isinstance(val, string_types):
+        return str(val)
+    return val
+
+
+def _format_repository_config(repo_id, repo_config):
+    to_be = configobj.ConfigObj()
+    to_be[repo_id] = {}
+    # Do basic translation of the items -> values
+    for (k, v) in repo_config.items():
+        # For now assume that people using this know the format
+        # of zypper repos  and don't verify keys/values further
+        to_be[repo_id][k] = _format_repo_value(v)
+    lines = to_be.write()
+    return "\n".join(lines)
+
+
+def _write_repos(repos, repo_base_path):
+    """Write the user-provided repo definition files
+    @param repos: A list of repo dictionary objects provided by the user's
+        cloud config.
+    @param repo_base_path: The directory path to which repo definitions are
+        written.
+    """
+
+    if not repos:
+        return
+    valid_repos = {}
+    for index, user_repo_config in enumerate(repos):
+        # Skip on absent required keys
+        missing_keys = set(['id', 'baseurl']).difference(set(user_repo_config))
+        if missing_keys:
+            LOG.warning(
+                "Repo config at index %d is missing required config keys: %s",
+                index, ",".join(missing_keys))
+            continue
+        repo_id = user_repo_config.get('id')
+        canon_repo_id = _canonicalize_id(repo_id)
+        repo_fn_pth = os.path.join(repo_base_path, "%s.repo" % (canon_repo_id))
+        if os.path.exists(repo_fn_pth):
+            LOG.info("Skipping repo %s, file %s already exists!",
+                     repo_id, repo_fn_pth)
+            continue
+        elif repo_id in valid_repos:
+            LOG.info("Skipping repo %s, file %s already pending!",
+                     repo_id, repo_fn_pth)
+            continue
+
+        # Do some basic key formatting
+        repo_config = dict(
+            (k.lower().strip().replace("-", "_"), v)
+            for k, v in user_repo_config.items()
+            if k and k != 'id')
+
+        # Set defaults if not present
+        for field in ['enabled', 'autorefresh']:
+            if field not in repo_config:
+                repo_config[field] = '1'
+
+        valid_repos[repo_id] = (repo_fn_pth, repo_config)
+
+    for (repo_id, repo_data) in valid_repos.items():
+        repo_blob = _format_repository_config(repo_id, repo_data[-1])
+        util.write_file(repo_data[0], repo_blob)
+
+
+def _write_zypp_config(zypper_config):
+    """Write to the default zypp configuration file /etc/zypp/zypp.conf"""
+    if not zypper_config:
+        return
+    zypp_config = '/etc/zypp/zypp.conf'
+    zypp_conf_content = util.load_file(zypp_config)
+    new_settings = ['# Added via cloud.cfg']
+    for setting, value in zypper_config.items():
+        if setting == 'configdir':
+            msg = 'Changing the location of the zypper configuration is '
+            msg += 'not supported, skipping "configdir" setting'
+            LOG.warning(msg)
+            continue
+        if value:
+            new_settings.append('%s=%s' % (setting, value))
+    if len(new_settings) > 1:
+        new_config = zypp_conf_content + '\n'.join(new_settings)
+    else:
+        new_config = zypp_conf_content
+    util.write_file(zypp_config, new_config)
+
+
+def handle(name, cfg, _cloud, log, _args):
+    zypper_section = cfg.get('zypper')
+    if not zypper_section:
+        LOG.debug(("Skipping module named %s,"
+                   " no 'zypper' relevant configuration found"), name)
+        return
+    repos = zypper_section.get('repos')
+    if not repos:
+        LOG.debug(("Skipping module named %s,"
+                   " no 'repos' configuration found"), name)
+        return
+    zypper_config = zypper_section.get('config', {})
+    repo_base_path = zypper_config.get('reposdir', '/etc/zypp/repos.d/')
+
+    _write_zypp_config(zypper_config)
+    _write_repos(repos, repo_base_path)
+
+# vi: ts=4 expandtab
diff --git a/config/cloud.cfg.tmpl b/config/cloud.cfg.tmpl
index 50e3bd8..32de9c9 100644
--- a/config/cloud.cfg.tmpl
+++ b/config/cloud.cfg.tmpl
@@ -84,6 +84,9 @@ cloud_config_modules:
  - apt-pipelining
  - apt-configure
 {% endif %}
+{% if variant in ["suse"] %}
+ - zypper-add-repo
+{% endif %}
 {% if variant not in ["freebsd"] %}
  - ntp
 {% endif %}
diff --git a/tests/unittests/test_handler/test_handler_zypper_add_repo.py b/tests/unittests/test_handler/test_handler_zypper_add_repo.py
new file mode 100644
index 0000000..315c2a5
--- /dev/null
+++ b/tests/unittests/test_handler/test_handler_zypper_add_repo.py
@@ -0,0 +1,237 @@
+# This file is part of cloud-init. See LICENSE file for license information.
+
+import glob
+import os
+
+from cloudinit.config import cc_zypper_add_repo
+from cloudinit import util
+
+from cloudinit.tests import helpers
+from cloudinit.tests.helpers import mock
+
+try:
+    from configparser import ConfigParser
+except ImportError:
+    from ConfigParser import ConfigParser
+import logging
+from six import StringIO
+
+LOG = logging.getLogger(__name__)
+
+
+class TestConfig(helpers.FilesystemMockingTestCase):
+    def setUp(self):
+        super(TestConfig, self).setUp()
+        self.tmp = self.tmp_dir()
+        self.zypp_conf = 'etc/zypp/zypp.conf'
+
+    def test_bad_repo_config(self):
+        """Config has no baseurl, no file should be written"""
+        cfg = {
+            'repos': [
+                {
+                    'id': 'foo',
+                    'name': 'suse-test',
+                    'enabled': '1'
+                },
+            ]
+        }
+        self.patchUtils(self.tmp)
+        cc_zypper_add_repo._write_repos(cfg['repos'], '/etc/zypp/repos.d')
+        self.assertRaises(IOError, util.load_file,
+                          "/etc/zypp/repos.d/foo.repo")
+
+    def test_write_repos(self):
+        """Verify valid repos get written"""
+        cfg = self._get_base_config_repos()
+        root_d = self.tmp_dir()
+        cc_zypper_add_repo._write_repos(cfg['zypper']['repos'], root_d)
+        repos = glob.glob('%s/*.repo' % root_d)
+        expected_repos = ['testing-foo.repo', 'testing-bar.repo']
+        if len(repos) != 2:
+            assert 'Number of repos written is "%d" expected 2' % len(repos)
+        for repo in repos:
+            repo_name = os.path.basename(repo)
+            if repo_name not in expected_repos:
+                assert 'Found repo with name "%s"; unexpected' % repo_name
+        # Validation that the content gets properly written is in another test
+
+    def test_write_repo(self):
+        """Verify the content of a repo file"""
+        cfg = {
+            'repos': [
+                {
+                    'baseurl': 'http://foo',
+                    'name': 'test-foo',
+                    'id': 'testing-foo'
+                },
+            ]
+        }
+        root_d = self.tmp_dir()
+        cc_zypper_add_repo._write_repos(cfg['repos'], root_d)
+        contents = util.load_file("%s/testing-foo.repo" % root_d)
+        parser = ConfigParser()
+        parser.readfp(StringIO(contents))
+        expected = {
+            'testing-foo': {
+                'name': 'test-foo',
+                'baseurl': 'http://foo',
+                'enabled': '1',
+                'autorefresh': '1'
+            }
+        }
+        for section in expected:
+            self.assertTrue(parser.has_section(section),
+                            "Contains section {0}".format(section))
+            for k, v in expected[section].items():
+                self.assertEqual(parser.get(section, k), v)
+
+    def test_config_write(self):
+        """Write valid configuration data"""
+        cfg = {
+            'config': {
+                'download.deltarpm': 'False',
+                'reposdir': 'foo'
+            }
+        }
+        root_d = self.tmp_dir()
+        helpers.populate_dir(root_d, {self.zypp_conf: '# Zypp config\n'})
+        self.reRoot(root_d)
+        cc_zypper_add_repo._write_zypp_config(cfg['config'])
+        cfg_out = os.path.join(root_d, self.zypp_conf)
+        contents = util.load_file(cfg_out)
+        expected = [
+            '# Zypp config',
+            '# Added via cloud.cfg',
+            'download.deltarpm=False',
+            'reposdir=foo'
+        ]
+        for item in contents.split('\n'):
+            if item not in expected:
+                self.assertIsNone(item)
+
+    @mock.patch('cloudinit.log.logging')
+    def test_config_write_skip_configdir(self, mock_logging):
+        """Write configuration but skip writing 'configdir' setting"""
+        cfg = {
+            'config': {
+                'download.deltarpm': 'False',
+                'reposdir': 'foo',
+                'configdir': 'bar'
+            }
+        }
+        root_d = self.tmp_dir()
+        helpers.populate_dir(root_d, {self.zypp_conf: '# Zypp config\n'})
+        self.reRoot(root_d)
+        cc_zypper_add_repo._write_zypp_config(cfg['config'])
+        cfg_out = os.path.join(root_d, self.zypp_conf)
+        contents = util.load_file(cfg_out)
+        expected = [
+            '# Zypp config',
+            '# Added via cloud.cfg',
+            'download.deltarpm=False',
+            'reposdir=foo'
+        ]
+        for item in contents.split('\n'):
+            if item not in expected:
+                self.assertIsNone(item)
+        # Not finding teh right path for mocking :(
+        # assert mock_logging.warning.called
+
+    def test_empty_config_section_no_new_data(self):
+        """When the config section is empty no new data should be written to
+           zypp.conf"""
+        cfg = self._get_base_config_repos()
+        cfg['zypper']['config'] = None
+        root_d = self.tmp_dir()
+        helpers.populate_dir(root_d, {self.zypp_conf: '# No data'})
+        self.reRoot(root_d)
+        cc_zypper_add_repo._write_zypp_config(cfg.get('config', {}))
+        cfg_out = os.path.join(root_d, self.zypp_conf)
+        contents = util.load_file(cfg_out)
+        self.assertEqual(contents, '# No data')
+
+    def test_empty_config_value_no_new_data(self):
+        """When the config section is not empty but there are no values
+           no new data should be written to zypp.conf"""
+        cfg = self._get_base_config_repos()
+        cfg['zypper']['config'] = {
+            'download.deltarpm': None
+        }
+        root_d = self.tmp_dir()
+        helpers.populate_dir(root_d, {self.zypp_conf: '# No data'})
+        self.reRoot(root_d)
+        cc_zypper_add_repo._write_zypp_config(cfg.get('config', {}))
+        cfg_out = os.path.join(root_d, self.zypp_conf)
+        contents = util.load_file(cfg_out)
+        self.assertEqual(contents, '# No data')
+
+    def test_handler_full_setup(self):
+        """Test that the handler ends up calling the renderers"""
+        cfg = self._get_base_config_repos()
+        cfg['zypper']['config'] = {
+            'download.deltarpm': 'False',
+        }
+        root_d = self.tmp_dir()
+        os.makedirs('%s/etc/zypp/repos.d' % root_d)
+        helpers.populate_dir(root_d, {self.zypp_conf: '# Zypp config\n'})
+        self.reRoot(root_d)
+        cc_zypper_add_repo.handle('zypper_add_repo', cfg, None, LOG, [])
+        cfg_out = os.path.join(root_d, self.zypp_conf)
+        contents = util.load_file(cfg_out)
+        expected = [
+            '# Zypp config',
+            '# Added via cloud.cfg',
+            'download.deltarpm=False',
+        ]
+        for item in contents.split('\n'):
+            if item not in expected:
+                self.assertIsNone(item)
+        repos = glob.glob('%s/etc/zypp/repos.d/*.repo' % root_d)
+        expected_repos = ['testing-foo.repo', 'testing-bar.repo']
+        if len(repos) != 2:
+            assert 'Number of repos written is "%d" expected 2' % len(repos)
+        for repo in repos:
+            repo_name = os.path.basename(repo)
+            if repo_name not in expected_repos:
+                assert 'Found repo with name "%s"; unexpected' % repo_name
+
+    def test_no_config_section_no_new_data(self):
+        """When there is no config section no new data should be written to
+           zypp.conf"""
+        cfg = self._get_base_config_repos()
+        root_d = self.tmp_dir()
+        helpers.populate_dir(root_d, {self.zypp_conf: '# No data'})
+        self.reRoot(root_d)
+        cc_zypper_add_repo._write_zypp_config(cfg.get('config', {}))
+        cfg_out = os.path.join(root_d, self.zypp_conf)
+        contents = util.load_file(cfg_out)
+        self.assertEqual(contents, '# No data')
+
+    def test_no_repo_data(self):
+        """When there is no repo data nothing should happen"""
+        root_d = self.tmp_dir()
+        self.reRoot(root_d)
+        cc_zypper_add_repo._write_repos(None, root_d)
+        content = glob.glob('%s/*' % root_d)
+        self.assertEqual(len(content), 0)
+
+    def _get_base_config_repos(self):
+        """Basic valid repo configuration"""
+        cfg = {
+            'zypper': {
+                'repos': [
+                    {
+                        'baseurl': 'http://foo',
+                        'name': 'test-foo',
+                        'id': 'testing-foo'
+                    },
+                    {
+                        'baseurl': 'http://bar',
+                        'name': 'test-bar',
+                        'id': 'testing-bar'
+                    }
+                ]
+            }
+        }
+        return cfg
diff --git a/tests/unittests/test_handler/test_schema.py b/tests/unittests/test_handler/test_schema.py
index 745bb0f..b8fc893 100644
--- a/tests/unittests/test_handler/test_schema.py
+++ b/tests/unittests/test_handler/test_schema.py
@@ -27,7 +27,13 @@ class GetSchemaTest(CiTestCase):
         """Every cloudconfig module with schema is listed in allOf keyword."""
         schema = get_schema()
         self.assertItemsEqual(
-            ['cc_bootcmd', 'cc_ntp', 'cc_resizefs', 'cc_runcmd'],
+            [
+                'cc_bootcmd',
+                'cc_ntp',
+                'cc_resizefs',
+                'cc_runcmd',
+                'cc_zypper_add_repo'
+            ],
             [subschema['id'] for subschema in schema['allOf']])
         self.assertEqual('cloud-config-schema', schema['id'])
         self.assertEqual(