From 33f2eacd88aa695ad23d355edd5301ef361dabb3 Mon Sep 17 00:00:00 2001 From: Luiz Carvalho Date: Mon, 23 Dec 2019 14:26:06 -0500 Subject: [PATCH] Use package_suffix instead of repository_suffix * OSBS-8412 Signed-off-by: Luiz Carvalho --- Dockerfile | 2 +- README.md | 9 ++-- omps/api/v1/push.py | 36 +++++++++++----- omps/quay.py | 32 ++++---------- setup.py | 2 +- tests/api/v1/test_api_push.py | 77 ++++++++++++++++++++++++++++++++++ tests/api/v2/test_api_push.py | 79 ++++++++++++++++++++++++++++++++++- tests/test_quay.py | 20 +++------ 8 files changed, 202 insertions(+), 55 deletions(-) diff --git a/Dockerfile b/Dockerfile index 0408d7b..54e8928 100644 --- a/Dockerfile +++ b/Dockerfile @@ -18,9 +18,9 @@ RUN dnf -y install \ python3-flask \ python3-jsonschema \ python3-koji \ - python3-pyyaml \ python3-requests \ python3-operator-courier \ + python3-ruamel-yaml \ && dnf -y clean all \ && rm -rf /tmp/* diff --git a/README.md b/README.md index a744309..6cfa2a1 100644 --- a/README.md +++ b/README.md @@ -101,13 +101,14 @@ Here's an example: ``` Replacements occur when pushing manifests into the application registry. -#### Altering repository names +#### Altering package names -Organizations can be configured so a suffix is appended to the repository name. -The suffix is only applied if the repository name does not already end with the suffix. +Organizations can be configured so a suffix is appended to the package and the repository names. +The suffix is only applied to the package if it does not already end with the suffix. +The repository is only modified if it's being taken from the packageName as in the v2 API. Example configuration: ``` -"repository_suffix": "-suffix" +"package_name_suffix": "-suffix" ``` ### Greenwave integration diff --git a/omps/api/v1/push.py b/omps/api/v1/push.py index 5d3d8b5..819f13e 100644 --- a/omps/api/v1/push.py +++ b/omps/api/v1/push.py @@ -10,7 +10,7 @@ import zipfile from flask import jsonify, current_app, request -from yaml import safe_load +from ruamel.yaml import YAML from . import API from omps.api.common import extract_auth_token, replace_registries @@ -166,19 +166,36 @@ def get_package_version(quay_org, repo, version=None): return version -def _get_reponame_from_manifests(source_dir): - for filename in sorted(os.listdir(source_dir)): - filename = os.path.join(source_dir, filename) +def _process_package_name(quay_org, dir_path): + """Returns package name from metadata + Retrieve the package name from the packageName attribute + of the package yaml file. + If not found, or malformed, raise PackageValidationError. + If package_name_suffix is configured, modify the packageName. + """ + yaml = YAML() + for filename in sorted(os.listdir(dir_path)): + filename = os.path.join(dir_path, filename) if filename.endswith('.yaml') or filename.endswith('.yml'): try: - with open(filename, 'r') as f: - contents = safe_load(f.read()) + with open(filename, 'r+') as f: + contents = yaml.load(f.read()) if 'packageName' in contents: name = contents['packageName'] logger.info("Found packageName %s in %s", name, filename) + if quay_org.package_name_suffix: + if not name.endswith(quay_org.package_name_suffix): + new_name = name + quay_org.package_name_suffix + contents['packageName'] = new_name + f.seek(0) + yaml.dump(contents, f) + f.truncate() + logger.info("Modified packageName %s in %s to %s", + name, filename, new_name) + name = new_name return name except Exception: - message = "Failed to parse yaml file %s" % filename[len(source_dir):] + message = "Failed to process yaml file %s" % filename[len(dir_path):] logger.exception(message) raise PackageValidationError(message) @@ -221,10 +238,9 @@ def _zip_flow(*, organization, repo, version, extract_manifest_func, logger.info("Extracted files: %s", extracted_files) data['extracted_files'] = extracted_files + package_name = _process_package_name(quay_org, tmpdir) if repo is None: - repo = _get_reponame_from_manifests(tmpdir) - - repo = quay_org.adjust_repository_name(repo) + repo = package_name version = get_package_version(quay_org, repo, version) logger.info("Using release version: %s", version) diff --git a/omps/quay.py b/omps/quay.py index 5e538d7..aabd59e 100644 --- a/omps/quay.py +++ b/omps/quay.py @@ -157,8 +157,8 @@ class OrgManager: "required": ["old", "new"], } }, - "repository_suffix": { - "description": "suffix to append to repository name", + "package_name_suffix": { + "description": "suffix to append to package name", "type": "string" } }, @@ -204,7 +204,7 @@ def get_org(self, organization, cnr_token): public=org_config.get('public', False), timeout=self._timeout, replace_registry_conf=org_config.get('replace_registry'), - repository_suffix=org_config.get('repository_suffix'), + package_name_suffix=org_config.get('package_name_suffix'), ) @@ -213,7 +213,7 @@ class QuayOrganization: def __init__( self, organization, cnr_token, oauth_token=None, public=False, - timeout=None, replace_registry_conf=None, repository_suffix=None + timeout=None, replace_registry_conf=None, package_name_suffix=None ): """ :param organization: organization name @@ -231,7 +231,7 @@ def __init__( self._public = public self._timeout = timeout self._replace_registry_conf = replace_registry_conf - self._repository_suffix = repository_suffix + self._package_name_suffix = package_name_suffix @property def public(self): @@ -245,6 +245,10 @@ def oauth_access(self): def organization(self): return self._organization + @property + def package_name_suffix(self): + return self._package_name_suffix + @property def registry_replacing_enabled(self): return bool(self._replace_registry_conf) @@ -277,24 +281,6 @@ def replace_registries(self, text): return text - def adjust_repository_name(self, repo): - """Returns the modified repository name - - The repository name is modified if there's a repository_suffix - configured for the Quay organization *and* the repository name - does not already end in the repository_suffix - - :param repo: name of repository - :rtype: str - :return: text with replaced registries - """ - if self._repository_suffix: - if not repo.endswith(self._repository_suffix): - self.logger.info("Applying repository suffix, %s, to %s", - self._repository_suffix, repo) - repo = repo + self._repository_suffix - return repo - def push_operator_manifest(self, repo, version, source_dir): """Build, verify and push operators artifact to quay.io registry diff --git a/setup.py b/setup.py index d52dc5e..29a08b1 100644 --- a/setup.py +++ b/setup.py @@ -60,9 +60,9 @@ def get_project_version(version_file='omps/__init__.py'): 'Flask==1.0.*', 'jsonschema', 'koji', - 'PyYAML', 'requests', 'operator-courier>=2.1.1', + 'ruamel.yaml', ], extras_require={ 'test': [ diff --git a/tests/api/v1/test_api_push.py b/tests/api/v1/test_api_push.py index 66b9889..e0de591 100644 --- a/tests/api/v1/test_api_push.py +++ b/tests/api/v1/test_api_push.py @@ -4,11 +4,16 @@ # from io import BytesIO +from textwrap import dedent +from unittest import mock +import os.path +import flexmock import requests import pytest from omps import constants +from omps.quay import QuayOrganization def test_push_zipfile( @@ -36,6 +41,78 @@ def test_push_zipfile( assert rv.get_json() == expected +@pytest.mark.parametrize(('suffix_func', 'expected_pkg_name_func'), ( + (lambda _: '-suffix', lambda x: x + '-suffix'), + (lambda x: x, lambda x: x), + (lambda x: '', lambda x: x), +)) +@mock.patch('omps.api.v1.push.ORG_MANAGER') +def test_push_zipfile_with_package_name_suffix( + mocked_org_manager, client, valid_manifests_archive, + endpoint_push_zipfile, mocked_quay_io, mocked_op_courier_push, + auth_header, suffix_func, expected_pkg_name_func): + """Test REST API for pushing operators form zipfile with package_name_suffix""" + original_pkg_name = valid_manifests_archive.pkg_name + expected_pkg_name = expected_pkg_name_func(original_pkg_name) + + mocked_org_manager.get_org.return_value = QuayOrganization( + endpoint_push_zipfile.org, 'cnr_token', + package_name_suffix=suffix_func(original_pkg_name)) + + def verify_modified_package_name(repo, version, source_dir): + # The modified YAML file should retain comments and defined order. The + # only modification should be the package name. + if valid_manifests_archive.pkg_name == 'marketplace': + pkg_manifest = ( + 'deploy/chart/catalog_resources/rh-operators/' + 'marketplace.v0.0.1.clusterserviceversion.yaml') + expected_packages_yaml = dedent("""\ + #! package-manifest: {pkg_manifest} + packageName: {pkg_name} + channels: + - name: alpha + currentCSV: marketplace-operator.v0.0.1 + """) + packages_yaml_path = os.path.join(source_dir, 'packages.yaml') + elif valid_manifests_archive.pkg_name == 'etcd': + pkg_manifest = ( + './deploy/chart/catalog_resources/rh-operators/' + 'etcdoperator.v0.9.2.clusterserviceversion.yaml') + expected_packages_yaml = dedent("""\ + #! package-manifest: {pkg_manifest} + packageName: {pkg_name} + channels: + - name: alpha + currentCSV: etcdoperator.v0.9.2 + """) + packages_yaml_path = os.path.join(source_dir, 'etcd.package.yaml') + else: + raise ValueError( + 'Unsupported manifests archive, {}'.format(valid_manifests_archive)) + expected_packages_yaml = expected_packages_yaml.format( + pkg_manifest=pkg_manifest, + pkg_name=expected_pkg_name) + with open(packages_yaml_path) as f: + assert f.read() == expected_packages_yaml + + flexmock(QuayOrganization)\ + .should_receive('push_operator_manifest')\ + .replace_with(verify_modified_package_name)\ + .once() + + with open(valid_manifests_archive.path, 'rb') as f: + rv = client.post( + endpoint_push_zipfile.url_path, + headers=auth_header, + data={'file': (f, f.name)}, + content_type='multipart/form-data', + ) + + assert rv.status_code == 200, rv.get_json() + # In V1, package_name_suffix does not change the repository name. + assert rv.get_json()['repo'] == endpoint_push_zipfile.repo + + @pytest.mark.parametrize('filename', ( 'test.json', # test invalid extension 'test.zip', # test invalid content diff --git a/tests/api/v2/test_api_push.py b/tests/api/v2/test_api_push.py index efbb3c5..ff55987 100644 --- a/tests/api/v2/test_api_push.py +++ b/tests/api/v2/test_api_push.py @@ -4,11 +4,16 @@ # from io import BytesIO +from textwrap import dedent +from unittest import mock +import os.path +import flexmock import requests import pytest from omps import constants +from omps.quay import QuayOrganization def test_push_zipfile( @@ -36,6 +41,78 @@ def test_push_zipfile( assert rv.get_json() == expected +@pytest.mark.parametrize(('suffix_func', 'expected_pkg_name_func'), ( + (lambda _: '-suffix', lambda x: x + '-suffix'), + (lambda x: x, lambda x: x), + (lambda x: '', lambda x: x), +)) +@mock.patch('omps.api.v1.push.ORG_MANAGER') +def test_push_zipfile_with_package_name_suffix( + mocked_org_manager, client, valid_manifests_archive, + endpoint_push_zipfile, mocked_quay_io, mocked_op_courier_push, + auth_header, suffix_func, expected_pkg_name_func): + """Test REST API for pushing operators form zipfile with package_name_suffix""" + original_pkg_name = valid_manifests_archive.pkg_name + expected_pkg_name = expected_pkg_name_func(original_pkg_name) + + mocked_org_manager.get_org.return_value = QuayOrganization( + endpoint_push_zipfile.org, 'cnr_token', + package_name_suffix=suffix_func(original_pkg_name)) + + def verify_modified_package_name(repo, version, source_dir): + # The modified YAML file should retain comments and defined order. The + # only modification should be the package name. + if valid_manifests_archive.pkg_name == 'marketplace': + pkg_manifest = ( + 'deploy/chart/catalog_resources/rh-operators/' + 'marketplace.v0.0.1.clusterserviceversion.yaml') + expected_packages_yaml = dedent("""\ + #! package-manifest: {pkg_manifest} + packageName: {pkg_name} + channels: + - name: alpha + currentCSV: marketplace-operator.v0.0.1 + """) + packages_yaml_path = os.path.join(source_dir, 'packages.yaml') + elif valid_manifests_archive.pkg_name == 'etcd': + pkg_manifest = ( + './deploy/chart/catalog_resources/rh-operators/' + 'etcdoperator.v0.9.2.clusterserviceversion.yaml') + expected_packages_yaml = dedent("""\ + #! package-manifest: {pkg_manifest} + packageName: {pkg_name} + channels: + - name: alpha + currentCSV: etcdoperator.v0.9.2 + """) + packages_yaml_path = os.path.join(source_dir, 'etcd.package.yaml') + else: + raise ValueError( + 'Unsupported manifests archive, {}'.format(valid_manifests_archive)) + expected_packages_yaml = expected_packages_yaml.format( + pkg_manifest=pkg_manifest, + pkg_name=expected_pkg_name) + with open(packages_yaml_path) as f: + assert f.read() == expected_packages_yaml + + flexmock(QuayOrganization)\ + .should_receive('push_operator_manifest')\ + .replace_with(verify_modified_package_name)\ + .once() + + with open(valid_manifests_archive.path, 'rb') as f: + rv = client.post( + endpoint_push_zipfile.url_path, + headers=auth_header, + data={'file': (f, f.name)}, + content_type='multipart/form-data', + ) + + assert rv.status_code == 200, rv.get_json() + # In V2, package_name_suffix also modifies the repository + assert rv.get_json()['repo'] == expected_pkg_name + + @pytest.mark.parametrize('filename', ( 'test.json', # test invalid extension 'test.zip', # test invalid content @@ -138,7 +215,7 @@ def test_push_invalid_manifests( assert rv_json['error'] == 'PackageValidationError' assert rv_json['message'] in ( 'Could not find packageName in manifests.', - 'Failed to parse yaml file /not_yaml.yaml', + 'Failed to process yaml file /not_yaml.yaml', ) diff --git a/tests/test_quay.py b/tests/test_quay.py index 1e3da56..d6471a3 100644 --- a/tests/test_quay.py +++ b/tests/test_quay.py @@ -513,17 +513,6 @@ def test_regexp_replace_registries(self, text, expected): qo = QuayOrganization(org, TOKEN, replace_registry_conf=replace_conf) assert qo.replace_registries(text) == expected - @pytest.mark.parametrize(('repo', 'suffix', 'expected'), ( - ('repo', '-suffix', 'repo-suffix'), - ('repo-suffix', '-suffix', 'repo-suffix'), - ('repo', '', 'repo'), - ('repo', None, 'repo'), - )) - def test_repository_suffix(self, repo, suffix, expected, organization): - """Test if repository_suffix property returns correct value""" - qo = QuayOrganization(organization, TOKEN, repository_suffix=suffix) - assert qo.adjust_repository_name(repo) == expected - class TestOrgManager: """Tets for OrgManager class""" @@ -549,8 +538,8 @@ class ConfClass: {'new': 'reg1', 'old': 'reg2'}, ] }, - 'org_with_repository_suffix': { - 'repository_suffix': '-suffix', + 'org_with_package_name_suffix': { + 'package_name_suffix': '-suffix', }, } @@ -583,8 +572,9 @@ class ConfClass: assert not priv_org_replacing_registries.oauth_access assert priv_org_replacing_registries.registry_replacing_enabled - org_with_repository_suffix = om.get_org('org_with_repository_suffix', 'cnr_token') - assert org_with_repository_suffix.adjust_repository_name('repo') == 'repo-suffix' + org_with_package_name_suffix = om.get_org('org_with_package_name_suffix', + 'cnr_token') + assert org_with_package_name_suffix.package_name_suffix == '-suffix' def test_getting_unconfigured_org(self): """Test of getting organization instance whne org is not configured in