diff --git a/docs/user-guide/release-notes/1.3.x.rst b/docs/user-guide/release-notes/1.3.x.rst new file mode 100644 index 00000000..7d7b213d --- /dev/null +++ b/docs/user-guide/release-notes/1.3.x.rst @@ -0,0 +1,31 @@ +============================= +Pulp OSTree 1.3 Release Notes +============================= + +Pulp OSTree 1.3.0 +================= + +New Features +------------ + +- The importer's algorithm for creating content units has been enhanced. + A content unit is created for each branch HEAD commit. The importer then walks up + the commit ancestry and creates content units for each parent commit. This ensure there + are no gaps in the branch history. The tree traversal ``depth`` may limit how much of the + history is available. The importer does not support cases where the remote repository has + used ``ostree reset`` to update a branch HEAD. + +- The CLI now supports the ``depth`` option which is used to configure the tree traversal + depth for both the importer and distributor. + + +API Changes +----------- + +None + + +Bugs Fixed +---------- + +You can see the :fixedbugs_pulp_ostree:`list of bugs fixed<1.3.0>`. diff --git a/docs/user-guide/release-notes/index.rst b/docs/user-guide/release-notes/index.rst index a714e33a..f4be74bd 100644 --- a/docs/user-guide/release-notes/index.rst +++ b/docs/user-guide/release-notes/index.rst @@ -9,3 +9,4 @@ Contents: 1.0.x 1.1.x 1.2.x + 1.3.x diff --git a/extensions_admin/pulp_ostree/extensions/admin/cudl.py b/extensions_admin/pulp_ostree/extensions/admin/cudl.py index 70ab4b4a..e017da1e 100644 --- a/extensions_admin/pulp_ostree/extensions/admin/cudl.py +++ b/extensions_admin/pulp_ostree/extensions/admin/cudl.py @@ -38,6 +38,10 @@ OPT_GPG_KEY = PulpCliOption( '--gpg-key', description, aliases=['-k'], required=False, allow_multiple=True) +description = _('Tree traversal depth. Default: 0') + +OPT_DEPTH = PulpCliOption('--depth', description, aliases=['-d'], required=False) + IMPORTER_CONFIGURATION_FLAGS = dict( include_ssl=True, @@ -75,6 +79,7 @@ def __init__(self, context): self.add_option(OPT_RELATIVE_PATH) self.add_option(OPT_BRANCH) self.add_option(OPT_GPG_KEY) + self.add_option(OPT_DEPTH) self.options_bundle.opt_feed.description = DESC_FEED def _describe_distributors(self, user_input): @@ -108,6 +113,11 @@ def _describe_distributors(self, user_input): constants.DISTRIBUTOR_CONFIG_KEY_RELATIVE_PATH: relative_path } + # traversal depth + depth = user_input.get(OPT_DEPTH.keyword) + if depth is not None: + config[constants.DISTRIBUTOR_CONFIG_KEY_DEPTH] = depth + data = [ dict(distributor_type_id=constants.WEB_DISTRIBUTOR_TYPE_ID, distributor_config=config, @@ -135,6 +145,9 @@ def _parse_importer_config(self, user_input): paths = user_input.pop(OPT_GPG_KEY.keyword, None) if paths: config[constants.IMPORTER_CONFIG_KEY_GPG_KEYS] = map(read, paths) + depth = user_input.get(OPT_DEPTH.keyword) + if depth is not None: + config[constants.IMPORTER_CONFIG_KEY_DEPTH] = depth return config @@ -146,6 +159,7 @@ def __init__(self, context): self.add_option(OPT_AUTO_PUBLISH) self.add_option(OPT_BRANCH) self.add_option(OPT_GPG_KEY) + self.add_option(OPT_DEPTH) self.options_bundle.opt_feed.description = DESC_FEED def run(self, **kwargs): @@ -153,6 +167,11 @@ def run(self, **kwargs): importer_config = self.parse_user_input(kwargs) + # traversal depth + if OPT_DEPTH.keyword in kwargs: + value = kwargs.get(OPT_DEPTH.keyword) + importer_config[constants.IMPORTER_CONFIG_KEY_DEPTH] = value + # branch list if OPT_BRANCH.keyword in kwargs: value = kwargs.pop(OPT_BRANCH.keyword) @@ -171,7 +190,8 @@ def run(self, **kwargs): # Remove importer specific keys for key in importer_config.keys(): - kwargs.pop(key, None) + if key not in (OPT_DEPTH.keyword,): + kwargs.pop(key, None) if importer_config: kwargs['importer_config'] = importer_config @@ -183,6 +203,11 @@ def run(self, **kwargs): if value is not None: web_config['auto_publish'] = value + # traversal depth + depth = kwargs.pop(OPT_DEPTH.keyword, None) + if depth is not None: + web_config[constants.DISTRIBUTOR_CONFIG_KEY_DEPTH] = depth + if web_config: kwargs['distributor_configs'] = {} kwargs['distributor_configs'][constants.CLI_WEB_DISTRIBUTOR_ID] = web_config diff --git a/plugins/pulp_ostree/plugins/distributors/configuration.py b/plugins/pulp_ostree/plugins/distributors/configuration.py index f3fc203b..6d291bd9 100644 --- a/plugins/pulp_ostree/plugins/distributors/configuration.py +++ b/plugins/pulp_ostree/plugins/distributors/configuration.py @@ -4,13 +4,11 @@ from pulp_ostree.common import constants -from mongoengine import Q -from pulp.server.db import model _LOG = logging.getLogger(__name__) -def validate_config(repo, config): +def validate_config(repo, config, conduit): """ Validate a configuration @@ -18,12 +16,14 @@ def validate_config(repo, config): :type repo: pulp.plugins.model.Repository :param config: Pulp configuration for the distributor :type config: pulp.plugins.config.PluginCallConfiguration + :param conduit: A configuration conduit. + :type conduit: pulp.plugins.conduits.repo_config.RepoConfigConduit :return: tuple of (bool, str) to describe the result :rtype: tuple """ repo_obj = repo.repo_obj relative_path = get_repo_relative_path(repo_obj, config) - error_msgs = _check_for_relative_path_conflicts(repo_obj.repo_id, relative_path) + error_msgs = _check_for_relative_path_conflicts(repo_obj.repo_id, relative_path, conduit) if error_msgs: return False, '\n'.join(error_msgs) @@ -98,7 +98,7 @@ def get_repo_relative_path(repo, config): return path -def _check_for_relative_path_conflicts(repo_id, relative_path): +def _check_for_relative_path_conflicts(repo_id, relative_path, conduit): """ Check that a relative path does not conflict with existing distributors' relative paths. @@ -106,32 +106,22 @@ def _check_for_relative_path_conflicts(repo_id, relative_path): :type repo_id: basestring :param relative_path: relative path of the repository :type relative_path: basestring - :return error_messages: a list of validation errors + :param conduit: A configuration conduit. + :type conduit: pulp.plugins.conduits.repo_config.RepoConfigConduit + :return: A list of validation error messages. :rtype: list """ - current_url_pieces = [x for x in relative_path.split('/') if x] - matching_url_list = [] - working_url = '' - for piece in current_url_pieces: - working_url = os.path.join(working_url, piece) - matching_url_list.append(working_url) - matching_url_list.append('/' + working_url) - - # Search for all the sub urls as well as any url that would fall within the specified url. - # The regex here basically matches the a url if it starts with (optional preceding slash) - # the working url. Anything can follow as long as it is separated by a slash. - rel_url_match = Q(config__relative_path={'$regex': '^/?' + working_url + '(/.*|/?\z)'}) - rel_url_in_list = Q(config__relative_path__in=matching_url_list) - - conflicts = model.Distributor.objects(rel_url_match | rel_url_in_list).only('repo_id', 'config') - error_messages = [] - for distributor in conflicts: + messages = [] + distributors = conduit.get_repo_distributors_by_relative_url(relative_path, repo_id) + for distributor in distributors: conflicting_repo_id = distributor['repo_id'] - conflicting_relative_url = None conflicting_relative_url = distributor['config']['relative_path'] - msg = _('Relative path [{relative_path}] for repository [{repo_id}] conflicts with ' - 'existing relative path [{conflict_url}] for repository [{conflict_repo}]') - error_messages.append(msg.format(relative_path=relative_path, repo_id=repo_id, - conflict_url=conflicting_relative_url, - conflict_repo=conflicting_repo_id)) - return error_messages + description = _('Relative path [{relative_path}] for repository [{repo_id}] conflicts ' + 'with relative path [{conflict_url}] for repository [{conflict_repo}]') + msg = description.format( + relative_path=relative_path, + repo_id=repo_id, + conflict_url=conflicting_relative_url, + conflict_repo=conflicting_repo_id) + messages.append(msg) + return messages diff --git a/plugins/pulp_ostree/plugins/distributors/web.py b/plugins/pulp_ostree/plugins/distributors/web.py index b99e318e..5b56c763 100644 --- a/plugins/pulp_ostree/plugins/distributors/web.py +++ b/plugins/pulp_ostree/plugins/distributors/web.py @@ -161,4 +161,4 @@ def validate_config(self, repo, config, config_conduit): :raises: PulpCodedValidationException if any validations failed """ - return configuration.validate_config(repo, config) + return configuration.validate_config(repo, config, config_conduit) diff --git a/plugins/pulp_ostree/plugins/importers/steps.py b/plugins/pulp_ostree/plugins/importers/steps.py index fb9f24c6..c6a29284 100644 --- a/plugins/pulp_ostree/plugins/importers/steps.py +++ b/plugins/pulp_ostree/plugins/importers/steps.py @@ -246,7 +246,7 @@ def __init__(self): def process_main(self, item=None): """ Find all branch (heads) in the local repository and - create content units for them. + create content units for each branch and commit in the history. """ lib_repository = lib.Repository(self.parent.storage_dir) for ref in lib_repository.list_refs(): @@ -257,16 +257,17 @@ def process_main(self, item=None): # not listed log.debug('skipping non-selected branch: {0}'.format(branch)) continue - unit = model.Branch( - remote_id=self.parent.remote_id, - branch=branch, - commit=ref.commit, - metadata=ref.metadata) - try: - unit.save() - except NotUniqueError: - unit = model.Branch.objects.get(**unit.unit_key) - associate_single_unit(self.get_repo().repo_obj, unit) + for commit in reversed(lib_repository.history(ref.commit)): + unit = model.Branch( + remote_id=self.parent.remote_id, + branch=branch, + commit=commit.id, + metadata=commit.metadata) + try: + unit.save() + except NotUniqueError: + unit = model.Branch.objects.get(**unit.unit_key) + associate_single_unit(self.get_repo().repo_obj, unit) class Clean(PluginStep): diff --git a/plugins/pulp_ostree/plugins/lib.py b/plugins/pulp_ostree/plugins/lib.py index 1d12145d..b24364c2 100644 --- a/plugins/pulp_ostree/plugins/lib.py +++ b/plugins/pulp_ostree/plugins/lib.py @@ -1,3 +1,4 @@ +from collections import namedtuple from logging import getLogger log = getLogger(__name__) @@ -127,6 +128,12 @@ def dict(self): return dict(self.__dict__) +# OSTree commit. +# id (str): The commit hash. +# metadata (dict): The commit metadata. +Commit = namedtuple('Commit', ['id', 'metadata']) + + class Variant(object): """ Variant encoding. @@ -385,6 +392,36 @@ def pull_local(self, path, refs, depth=0): self.open() self.impl.pull_with_options(url, Variant.opt_dict(options), None, None) + def history(self, commit_id): + """ + Get commit history. + Traversal of the commit hierarchy. + Depending on the traversal 'depth' setting, the entire commit + hierarchy may not have been pulled. This is detected when GError + is raised. Unfortunately, a more specific exception is not raised. + + :param commit_id: A commit (hash) used as the starting point for the traversal. + :type commit_id: str + :return: A list of: Commit. + :rtype: list + """ + lib = Lib() + self.open() + history = [] + while commit_id: + try: + _, commit = self.impl.load_variant(lib.OSTree.ObjectType.COMMIT, commit_id) + except lib.GLib.GError as le: + if history: + # parent not pulled. + log.debug(le) + break + else: + raise + history.append(Commit(id=commit_id, metadata=commit[0])) + commit_id = lib.OSTree.commit_get_parent(commit) + return history + class Remote(object): """ diff --git a/plugins/test/unit/plugins/distributors/test_configuration.py b/plugins/test/unit/plugins/distributors/test_configuration.py index 789b1e6f..73b2477c 100644 --- a/plugins/test/unit/plugins/distributors/test_configuration.py +++ b/plugins/test/unit/plugins/distributors/test_configuration.py @@ -49,11 +49,12 @@ def test_get_repo_relative_path_when_passed(self): self.assertEquals(directory, relative_path[1:]) -@mock.patch('pulp_ostree.plugins.distributors.configuration.model.Distributor.objects') class TestValidateConfig(unittest.TestCase): - def test_server_url_fully_qualified(self, mock_dist_qs): + def test_server_url_fully_qualified(self): m_repo = mock.MagicMock() + conduit = mock.MagicMock() + conduit.get_repo_distributors_by_relative_url.return_value = [] config = PluginCallConfiguration({}, {}) self.assertEquals( - (True, None), configuration.validate_config(m_repo, config)) + (True, None), configuration.validate_config(m_repo, config, conduit)) diff --git a/plugins/test/unit/plugins/distributors/test_web.py b/plugins/test/unit/plugins/distributors/test_web.py index 4958697a..282b0b87 100644 --- a/plugins/test/unit/plugins/distributors/test_web.py +++ b/plugins/test/unit/plugins/distributors/test_web.py @@ -84,6 +84,8 @@ def test_cancel_publish_repo(self): @patch('pulp_ostree.plugins.distributors.web.configuration.validate_config') def test_validate_config(self, mock_validate): m_repo = Mock() - value = self.distributor.validate_config(m_repo, 'foo', Mock()) - mock_validate.assert_called_once_with(m_repo, 'foo') + conduit = Mock() + repository = Mock() + value = self.distributor.validate_config(m_repo, repository, conduit) + mock_validate.assert_called_once_with(m_repo, repository, conduit) self.assertEquals(value, mock_validate.return_value) diff --git a/plugins/test/unit/plugins/importers/test_steps.py b/plugins/test/unit/plugins/importers/test_steps.py index 0d0e96b2..f102daff 100644 --- a/plugins/test/unit/plugins/importers/test_steps.py +++ b/plugins/test/unit/plugins/importers/test_steps.py @@ -2,14 +2,14 @@ from pulp.common.compat import unittest -from mock import patch, Mock, PropertyMock, ANY +from mock import patch, Mock, PropertyMock, ANY, call from pulp.common.plugins import importer_constants from pulp.server.exceptions import PulpCodedException from mongoengine import NotUniqueError -from pulp_ostree.plugins.lib import LibError +from pulp_ostree.plugins.lib import LibError, Commit from pulp_ostree.plugins.importers.steps import ( Main, Create, Summary, Pull, Add, Clean, Remote, Repair) from pulp_ostree.common import constants, errors @@ -275,6 +275,12 @@ def test_init(self): @patch(MODULE + '.model') @patch(MODULE + '.associate_single_unit') def test_process_main(self, fake_associate, fake_model, fake_lib): + def history(commit_id): + return [ + Commit(id='{}head'.format(commit_id), metadata={'md': 0}), + Commit(id='{}parent-1'.format(commit_id), metadata={'md': 1}), + Commit(id='{}parent-2'.format(commit_id), metadata={'md': 2}), + ] repo_id = 'r-1234' remote_id = 'remote-1' refs = [ @@ -284,7 +290,15 @@ def test_process_main(self, fake_associate, fake_model, fake_lib): Mock(path='branch:4', commit='commit:4', metadata='md:4'), Mock(path='branch:5', commit='commit:5', metadata='md:5'), ] - units = [Mock(ref=r, unit_key={}) for r in refs] + + units = [ + Mock(remote_id=remote_id, + branch=r.path.split(':')[-1], + commit=c.id, + metadata=c.metadata, + unit_key={}) for r in refs[:-1] for c in reversed(history(r.commit)) + ] + units[0].save.side_effect = NotUniqueError # duplicate fake_model.Branch.side_effect = units @@ -294,6 +308,7 @@ def test_process_main(self, fake_associate, fake_model, fake_lib): repository = Mock() repository.list_refs.return_value = refs + repository.history.side_effect = history fake_lib.Repository.return_value = repository parent = Mock(remote_id=remote_id, storage_dir='/tmp/xyz', branches=branches) @@ -312,17 +327,15 @@ def test_process_main(self, fake_associate, fake_model, fake_lib): self.assertEqual( fake_model.Branch.call_args_list, [ - ((), dict( - remote_id=remote_id, - branch=r.path.split(':')[-1], - commit=r.commit, - metadata=r.metadata)) - for r in refs[:-1] + call(remote_id=u.remote_id, + branch=u.branch, + commit=u.commit, + metadata=u.metadata) for u in units ]) self.assertEqual( fake_associate.call_args_list, [ - ((parent.get_repo.return_value.repo_obj, u), {}) for u in units[:-1] + ((parent.get_repo.return_value.repo_obj, u), {}) for u in units ]) diff --git a/plugins/test/unit/plugins/test_lib.py b/plugins/test/unit/plugins/test_lib.py index 4b30baea..8dd64822 100644 --- a/plugins/test/unit/plugins/test_lib.py +++ b/plugins/test/unit/plugins/test_lib.py @@ -6,6 +6,7 @@ Lib, LibError, ProgressReport, + Commit, Ref, Remote, Variant, @@ -519,6 +520,88 @@ def test_pull_finished_always_called(self, lib): self.assertRaises(LibError, repo.pull, '', [], None) progress.finish.assert_called_once_with() + @patch('pulp_ostree.plugins.lib.Lib') + def test_history(self, lib): + commit_id = '123' + parents = [ + '456', + '789', + None + ] + variants = [ + (True, ({'version': 1},)), + (True, ({'version': 2},)), + (True, ({'version': 3},)), + ] + _repo = Mock() + _repo.load_variant.side_effect = variants + _lib = Mock() + _lib.OSTree.Repo.new.return_value = _repo + _lib.GLib.GError = GError + _lib.OSTree.ObjectType.COMMIT = 'COMMIT' + _lib.OSTree.commit_get_parent.side_effect = parents + lib.return_value = _lib + + # test + repo = Repository('') + history = repo.history(commit_id) + + # validation + self.assertEqual(history, [ + Commit('123', {'version': 1}), + Commit('456', {'version': 2}), + Commit('789', {'version': 3}), + ]) + + @patch('pulp_ostree.plugins.lib.Lib') + def test_history_not_pulled(self, lib): + # Depending on tree traversal depth, the entire commit + # hierarchy may not have been pulled. + commit_id = '123' + parents = [ + '456', + '789', + None + ] + variants = [ + (True, ({'version': 1},)), + (True, ({'version': 2},)), + GError, + ] + _repo = Mock() + _repo.load_variant.side_effect = variants + _lib = Mock() + _lib.OSTree.Repo.new.return_value = _repo + _lib.GLib.GError = GError + _lib.OSTree.ObjectType.COMMIT = 'COMMIT' + _lib.OSTree.commit_get_parent.side_effect = parents + lib.return_value = _lib + + # test + repo = Repository('') + history = repo.history(commit_id) + + # validation + self.assertEqual(history, [ + Commit('123', {'version': 1}), + Commit('456', {'version': 2}), + ]) + + @patch('pulp_ostree.plugins.lib.Lib') + def test_history_failed(self, lib): + commit_id = '123' + _repo = Mock() + _repo.load_variant.side_effect = GError + _lib = Mock() + _lib.OSTree.Repo.new.return_value = _repo + _lib.GLib.GError = GError + _lib.OSTree.ObjectType.COMMIT = 'COMMIT' + lib.return_value = _lib + + # test + repo = Repository('') + self.assertRaises(GError, repo.history, commit_id) + class TestRemote(TestCase):