[PATCHv2 0/3] Fix repository related testcases
From: Royce Lv <lvroyce@linux.vnet.ibm.com> Repository update remove valid repo but when add fails did not add back. And url need to check so fake ones will result error. repository config params need to be check. Royce Lv (3): Fix: Avoid passing unexpected config params to repository manager issue#433: Fix fake url for repository test Fix: Add rollback to update repository docs/API.md | 2 ++ src/kimchi/API.json | 12 ++++++++++- src/kimchi/i18n.py | 1 + src/kimchi/repositories.py | 52 +++++++++++++++++++++++++++++----------------- tests/test_model.py | 41 ++++++++++++++++++++++++------------ 5 files changed, 75 insertions(+), 33 deletions(-) -- 1.8.3.2
From: Royce Lv <lvroyce@linux.vnet.ibm.com> We allow config params for both yum and apt in API.json, while we need to filter out parmas not proper for each kind. So add this logic to repository model. Also fix gpgkey handling for yum. Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- docs/API.md | 2 ++ src/kimchi/API.json | 12 +++++++++++- src/kimchi/i18n.py | 1 + src/kimchi/repositories.py | 20 +++++++++++++++----- tests/test_model.py | 22 ++++++++++++++++++---- 5 files changed, 47 insertions(+), 10 deletions(-) diff --git a/docs/API.md b/docs/API.md index b679ce7..a6a2bcc 100644 --- a/docs/API.md +++ b/docs/API.md @@ -963,6 +963,8 @@ http://, ftp:// or file:// URL. list of baseurls for YUM repository * dist: Distribution to DEB repository * comps *(optional)*: List of components to DEB repository + * gpgcheck *(optional)*: Indicates if a GPG signature check on the packages got from repository should be performed. + * gpgkey *(optional)*: URL pointing to the ASCII-armored GPG key file for the repository. ### Resource: Repository diff --git a/src/kimchi/API.json b/src/kimchi/API.json index b8604d2..43d731e 100644 --- a/src/kimchi/API.json +++ b/src/kimchi/API.json @@ -666,6 +666,16 @@ "description": "URL to a file containing a list of baseurls", "type": "string", "error": "KCHREPOS0007E" + }, + "gpgcheck": { + "description": "Indicates if a GPG signature check on the packages got from repository should be performed.", + "type": "boolean", + "error": "KCHREPOS0009E" + }, + "gpgkey": { + "description": "URL pointing to the ASCII-armored GPG key file for the repository.", + "type": "string", + "error": "KCHREPOS0010E" } } } @@ -713,7 +723,7 @@ "error": "KCHREPOS0007E" }, "gpgcheck": { - "description": "Indicates if a GPG signature check on the packages gotten from repository should be performed.", + "description": "Indicates if a GPG signature check on the packages got from repository should be performed.", "type": "boolean", "error": "KCHREPOS0009E" }, diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py index 9e66c68..e83843c 100644 --- a/src/kimchi/i18n.py +++ b/src/kimchi/i18n.py @@ -287,4 +287,5 @@ messages = { "KCHREPOS0025E": _("Unable to retrieve repository information. Details: '%(err)s'"), "KCHREPOS0026E": _("Unable to add repository. Details: '%(err)s'"), "KCHREPOS0027E": _("Unable to remove repository. Details: '%(err)s'"), + "KCHREPOS0028E": _("Configuration items: '%(items)s' are not supported by repository manager"), } diff --git a/src/kimchi/repositories.py b/src/kimchi/repositories.py index 676b7c2..3ac0e9c 100644 --- a/src/kimchi/repositories.py +++ b/src/kimchi/repositories.py @@ -25,7 +25,7 @@ from ConfigParser import ConfigParser from kimchi.basemodel import Singleton from kimchi.config import kimchiLock -from kimchi.exception import InvalidOperation +from kimchi.exception import InvalidOperation, InvalidParameter from kimchi.exception import OperationFailed, NotFoundError, MissingParameter from kimchi.utils import validate_repo_url @@ -104,6 +104,7 @@ class YumRepo(object): """ TYPE = 'yum' DEFAULT_CONF_DIR = "/etc/yum.repos.d" + CONFIG_ENTRY = ('repo_name', 'mirrorlist', 'gpgcheck', 'gpgkey') def __init__(self): self._yb = getattr(__import__('yum'), 'YumBase') @@ -172,8 +173,11 @@ class YumRepo(object): """ # At least one base url, or one mirror, must be given. baseurl = params.get('baseurl', '') - config = params.get('config', {}) + extra_keys = list(set(config.keys()).difference(set(self.CONFIG_ENTRY))) + if len(extra_keys) > 0: + raise InvalidParameter("KCHREPOS0028E", + {'items': ",".join(extra_keys)}) mirrorlist = config.get('mirrorlist', '') if not baseurl and not mirrorlist: raise MissingParameter("KCHREPOS0013E") @@ -195,9 +199,11 @@ class YumRepo(object): raise InvalidOperation("KCHREPOS0022E", {'repo_id': repo_id}) repo_name = config.get('repo_name', repo_id) + gpgcheck = config.get('gpgcheck', 1) + gpgkey = config.get('gpgkey', list()) repo = {'baseurl': baseurl, 'mirrorlist': mirrorlist, - 'name': repo_name, 'gpgcheck': 1, - 'gpgkey': [], 'enabled': 1} + 'name': repo_name, 'gpgcheck': gpgcheck, + 'gpgkey': gpgkey, 'enabled': 1} # write a repo file in the system with repo{} information. parser = ConfigParser() @@ -318,6 +324,7 @@ class AptRepo(object): """ TYPE = 'deb' KIMCHI_LIST = "kimchi-source.list" + CONFIG_ENTRY = ('dist', 'comps') def __init__(self): getattr(__import__('apt_pkg'), 'init_config')() @@ -413,12 +420,15 @@ class AptRepo(object): # To create a APT repository the dist is a required parameter # (in addition to baseurl, verified on controller through API.json) config = params.get('config', None) + extra_keys = list(set(config.keys()).difference(set(self.CONFIG_ENTRY))) + if len(extra_keys) > 0: + raise InvalidParameter("KCHREPOS0028E", + {'items': ",".join(extra_keys)}) if config is None: raise MissingParameter("KCHREPOS0019E") if 'dist' not in config.keys(): raise MissingParameter("KCHREPOS0019E") - uri = params['baseurl'] dist = config['dist'] comps = config.get('comps', []) diff --git a/tests/test_model.py b/tests/test_model.py index ceedc6f..56a8e12 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -1295,11 +1295,21 @@ class ModelTests(unittest.TestCase): {'baseurl': 'http://br.archive.kimchi.com/ubuntu/fake', 'config': {'dist': 'quantal', 'comps': ['main']}}] + invalid_deb_repo = {'baseurl': 'http://br.archive.ubuntu.com/ubuntu/', + 'config': {'gpgkey': 'file:///fake-key-file'}} + + invalid_yum_repo = { + 'repo_id': 'fedora-fake2', + 'baseurl': 'http://www.fedora.org', + 'config': {'dist': 'quantal', 'comps': ['main']}} + repo_type = inst.capabilities_lookup()['repo_mngt_tool'] if repo_type == 'yum': test_repos = yum_repos + invalid_repo = invalid_deb_repo elif repo_type == 'deb': test_repos = deb_repos + invalid_repo = invalid_yum_repo else: # repository management tool was not recognized by Kimchi # skip test case @@ -1317,11 +1327,15 @@ class ModelTests(unittest.TestCase): 'config': {'dist': 'quantal'}} self.assertRaises(InvalidParameter, inst.repositories_create, repo) + # Create repo with invalid config + self.assertRaises(InvalidParameter, inst.repositories_create, repo) + # create repositories with invalid mirrorlist - for url in invalid_urls: - repo = {'repo_id': 'repo_fake', - 'config': {'mirrorlist': url, 'dist': 'quantal'}} - self.assertRaises(InvalidParameter, inst.repositories_create, repo) + if repo_type == 'yum': + for url in invalid_urls: + repo = {'repo_id': 'repo_fake', + 'config': {'mirrorlist': url, 'dist': 'quantal'}} + self.assertRaises(InvalidParameter, inst.repositories_create, repo) for repo in test_repos: system_host_repos = len(inst.repositories_get_list()) -- 1.8.3.2
On 09/22/2014 06:32 AM, lvroyce@linux.vnet.ibm.com wrote:
From: Royce Lv <lvroyce@linux.vnet.ibm.com>
We allow config params for both yum and apt in API.json, while we need to filter out parmas not proper for each kind. So add this logic to repository model. Also fix gpgkey handling for yum.
Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- docs/API.md | 2 ++ src/kimchi/API.json | 12 +++++++++++- src/kimchi/i18n.py | 1 + src/kimchi/repositories.py | 20 +++++++++++++++----- tests/test_model.py | 22 ++++++++++++++++++---- 5 files changed, 47 insertions(+), 10 deletions(-)
diff --git a/docs/API.md b/docs/API.md index b679ce7..a6a2bcc 100644 --- a/docs/API.md +++ b/docs/API.md @@ -963,6 +963,8 @@ http://, ftp:// or file:// URL. list of baseurls for YUM repository * dist: Distribution to DEB repository * comps *(optional)*: List of components to DEB repository + * gpgcheck *(optional)*: Indicates if a GPG signature check on the packages got from repository should be performed. + * gpgkey *(optional)*: URL pointing to the ASCII-armored GPG key file for the repository.
### Resource: Repository
diff --git a/src/kimchi/API.json b/src/kimchi/API.json index b8604d2..43d731e 100644 --- a/src/kimchi/API.json +++ b/src/kimchi/API.json @@ -666,6 +666,16 @@ "description": "URL to a file containing a list of baseurls", "type": "string", "error": "KCHREPOS0007E" + }, + "gpgcheck": { + "description": "Indicates if a GPG signature check on the packages got from repository should be performed.", + "type": "boolean", + "error": "KCHREPOS0009E" + }, + "gpgkey": { + "description": "URL pointing to the ASCII-armored GPG key file for the repository.", + "type": "string", + "error": "KCHREPOS0010E" } } } @@ -713,7 +723,7 @@ "error": "KCHREPOS0007E" }, "gpgcheck": { - "description": "Indicates if a GPG signature check on the packages gotten from repository should be performed.", + "description": "Indicates if a GPG signature check on the packages got from repository should be performed.", "type": "boolean", "error": "KCHREPOS0009E" }, diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py index 9e66c68..e83843c 100644 --- a/src/kimchi/i18n.py +++ b/src/kimchi/i18n.py @@ -287,4 +287,5 @@ messages = { "KCHREPOS0025E": _("Unable to retrieve repository information. Details: '%(err)s'"), "KCHREPOS0026E": _("Unable to add repository. Details: '%(err)s'"), "KCHREPOS0027E": _("Unable to remove repository. Details: '%(err)s'"), + "KCHREPOS0028E": _("Configuration items: '%(items)s' are not supported by repository manager"), } diff --git a/src/kimchi/repositories.py b/src/kimchi/repositories.py index 676b7c2..3ac0e9c 100644 --- a/src/kimchi/repositories.py +++ b/src/kimchi/repositories.py @@ -25,7 +25,7 @@ from ConfigParser import ConfigParser
from kimchi.basemodel import Singleton from kimchi.config import kimchiLock -from kimchi.exception import InvalidOperation +from kimchi.exception import InvalidOperation, InvalidParameter from kimchi.exception import OperationFailed, NotFoundError, MissingParameter from kimchi.utils import validate_repo_url
@@ -104,6 +104,7 @@ class YumRepo(object): """ TYPE = 'yum' DEFAULT_CONF_DIR = "/etc/yum.repos.d" + CONFIG_ENTRY = ('repo_name', 'mirrorlist', 'gpgcheck', 'gpgkey')
def __init__(self): self._yb = getattr(__import__('yum'), 'YumBase') @@ -172,8 +173,11 @@ class YumRepo(object): """ # At least one base url, or one mirror, must be given. baseurl = params.get('baseurl', '') - config = params.get('config', {}) + extra_keys = list(set(config.keys()).difference(set(self.CONFIG_ENTRY))) + if len(extra_keys) > 0: + raise InvalidParameter("KCHREPOS0028E", + {'items': ",".join(extra_keys)}) mirrorlist = config.get('mirrorlist', '') if not baseurl and not mirrorlist: raise MissingParameter("KCHREPOS0013E") @@ -195,9 +199,11 @@ class YumRepo(object): raise InvalidOperation("KCHREPOS0022E", {'repo_id': repo_id})
repo_name = config.get('repo_name', repo_id)
+ gpgcheck = config.get('gpgcheck', 1)
From jsonschema, gpgcheck is boolean, ie, true or false, and the yum repo config expects 1 or 0
+ gpgkey = config.get('gpgkey', list())
From jsonschema, gpgkey is a string and the yum repo config expects a list I know I have commented that gpgcheck and gpgkey were missing on docs/API.md but they are supposed to be like that as during the repo creation we assume the defaut values and they can be changed on PUT request
repo = {'baseurl': baseurl, 'mirrorlist': mirrorlist, - 'name': repo_name, 'gpgcheck': 1, - 'gpgkey': [], 'enabled': 1} + 'name': repo_name, 'gpgcheck': gpgcheck, + 'gpgkey': gpgkey, 'enabled': 1}
# write a repo file in the system with repo{} information. parser = ConfigParser() @@ -318,6 +324,7 @@ class AptRepo(object): """ TYPE = 'deb' KIMCHI_LIST = "kimchi-source.list" + CONFIG_ENTRY = ('dist', 'comps')
def __init__(self): getattr(__import__('apt_pkg'), 'init_config')() @@ -413,12 +420,15 @@ class AptRepo(object): # To create a APT repository the dist is a required parameter # (in addition to baseurl, verified on controller through API.json) config = params.get('config', None) + extra_keys = list(set(config.keys()).difference(set(self.CONFIG_ENTRY))) + if len(extra_keys) > 0: + raise InvalidParameter("KCHREPOS0028E", + {'items': ",".join(extra_keys)}) if config is None: raise MissingParameter("KCHREPOS0019E")
if 'dist' not in config.keys(): raise MissingParameter("KCHREPOS0019E") - uri = params['baseurl'] dist = config['dist'] comps = config.get('comps', []) diff --git a/tests/test_model.py b/tests/test_model.py index ceedc6f..56a8e12 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -1295,11 +1295,21 @@ class ModelTests(unittest.TestCase): {'baseurl': 'http://br.archive.kimchi.com/ubuntu/fake', 'config': {'dist': 'quantal', 'comps': ['main']}}]
+ invalid_deb_repo = {'baseurl': 'http://br.archive.ubuntu.com/ubuntu/', + 'config': {'gpgkey': 'file:///fake-key-file'}} + + invalid_yum_repo = { + 'repo_id': 'fedora-fake2', + 'baseurl': 'http://www.fedora.org', + 'config': {'dist': 'quantal', 'comps': ['main']}} + repo_type = inst.capabilities_lookup()['repo_mngt_tool'] if repo_type == 'yum': test_repos = yum_repos + invalid_repo = invalid_deb_repo elif repo_type == 'deb': test_repos = deb_repos + invalid_repo = invalid_yum_repo else: # repository management tool was not recognized by Kimchi # skip test case @@ -1317,11 +1327,15 @@ class ModelTests(unittest.TestCase): 'config': {'dist': 'quantal'}} self.assertRaises(InvalidParameter, inst.repositories_create, repo)
+ # Create repo with invalid config + self.assertRaises(InvalidParameter, inst.repositories_create, repo) + # create repositories with invalid mirrorlist - for url in invalid_urls: - repo = {'repo_id': 'repo_fake', - 'config': {'mirrorlist': url, 'dist': 'quantal'}} - self.assertRaises(InvalidParameter, inst.repositories_create, repo)
+ if repo_type == 'yum': + for url in invalid_urls: + repo = {'repo_id': 'repo_fake', + 'config': {'mirrorlist': url, 'dist': 'quantal'}} + self.assertRaises(InvalidParameter, inst.repositories_create, repo)
You are restricting the test for yum repos. You should create the repo params according to repo type to test deb and yum for url in invalid_urls: if repo_type == 'yum': repo = {} elif repo_type == 'deb': repo = {} self.assertRaises(...)
for repo in test_repos: system_host_repos = len(inst.repositories_get_list())
On 2014年09月22日 23:12, Aline Manera wrote:
On 09/22/2014 06:32 AM, lvroyce@linux.vnet.ibm.com wrote:
From: Royce Lv <lvroyce@linux.vnet.ibm.com>
We allow config params for both yum and apt in API.json, while we need to filter out parmas not proper for each kind. So add this logic to repository model. Also fix gpgkey handling for yum.
Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- docs/API.md | 2 ++ src/kimchi/API.json | 12 +++++++++++- src/kimchi/i18n.py | 1 + src/kimchi/repositories.py | 20 +++++++++++++++----- tests/test_model.py | 22 ++++++++++++++++++---- 5 files changed, 47 insertions(+), 10 deletions(-)
diff --git a/docs/API.md b/docs/API.md index b679ce7..a6a2bcc 100644 --- a/docs/API.md +++ b/docs/API.md @@ -963,6 +963,8 @@ http://, ftp:// or file:// URL. list of baseurls for YUM repository * dist: Distribution to DEB repository * comps *(optional)*: List of components to DEB repository + * gpgcheck *(optional)*: Indicates if a GPG signature check on the packages got from repository should be performed. + * gpgkey *(optional)*: URL pointing to the ASCII-armored GPG key file for the repository.
### Resource: Repository
diff --git a/src/kimchi/API.json b/src/kimchi/API.json index b8604d2..43d731e 100644 --- a/src/kimchi/API.json +++ b/src/kimchi/API.json @@ -666,6 +666,16 @@ "description": "URL to a file containing a list of baseurls", "type": "string", "error": "KCHREPOS0007E" + }, + "gpgcheck": { + "description": "Indicates if a GPG signature check on the packages got from repository should be performed.", + "type": "boolean", + "error": "KCHREPOS0009E" + }, + "gpgkey": { + "description": "URL pointing to the ASCII-armored GPG key file for the repository.", + "type": "string", + "error": "KCHREPOS0010E" } } } @@ -713,7 +723,7 @@ "error": "KCHREPOS0007E" }, "gpgcheck": { - "description": "Indicates if a GPG signature check on the packages gotten from repository should be performed.", + "description": "Indicates if a GPG signature check on the packages got from repository should be performed.", "type": "boolean", "error": "KCHREPOS0009E" }, diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py index 9e66c68..e83843c 100644 --- a/src/kimchi/i18n.py +++ b/src/kimchi/i18n.py @@ -287,4 +287,5 @@ messages = { "KCHREPOS0025E": _("Unable to retrieve repository information. Details: '%(err)s'"), "KCHREPOS0026E": _("Unable to add repository. Details: '%(err)s'"), "KCHREPOS0027E": _("Unable to remove repository. Details: '%(err)s'"), + "KCHREPOS0028E": _("Configuration items: '%(items)s' are not supported by repository manager"), } diff --git a/src/kimchi/repositories.py b/src/kimchi/repositories.py index 676b7c2..3ac0e9c 100644 --- a/src/kimchi/repositories.py +++ b/src/kimchi/repositories.py @@ -25,7 +25,7 @@ from ConfigParser import ConfigParser
from kimchi.basemodel import Singleton from kimchi.config import kimchiLock -from kimchi.exception import InvalidOperation +from kimchi.exception import InvalidOperation, InvalidParameter from kimchi.exception import OperationFailed, NotFoundError, MissingParameter from kimchi.utils import validate_repo_url
@@ -104,6 +104,7 @@ class YumRepo(object): """ TYPE = 'yum' DEFAULT_CONF_DIR = "/etc/yum.repos.d" + CONFIG_ENTRY = ('repo_name', 'mirrorlist', 'gpgcheck', 'gpgkey')
def __init__(self): self._yb = getattr(__import__('yum'), 'YumBase') @@ -172,8 +173,11 @@ class YumRepo(object): """ # At least one base url, or one mirror, must be given. baseurl = params.get('baseurl', '') - config = params.get('config', {}) + extra_keys = list(set(config.keys()).difference(set(self.CONFIG_ENTRY))) + if len(extra_keys) > 0: + raise InvalidParameter("KCHREPOS0028E", + {'items': ",".join(extra_keys)}) mirrorlist = config.get('mirrorlist', '') if not baseurl and not mirrorlist: raise MissingParameter("KCHREPOS0013E") @@ -195,9 +199,11 @@ class YumRepo(object): raise InvalidOperation("KCHREPOS0022E", {'repo_id': repo_id})
repo_name = config.get('repo_name', repo_id)
+ gpgcheck = config.get('gpgcheck', 1)
From jsonschema, gpgcheck is boolean, ie, true or false, and the yum repo config expects 1 or 0
+ gpgkey = config.get('gpgkey', list())
From jsonschema, gpgkey is a string and the yum repo config expects a list
I know I have commented that gpgcheck and gpgkey were missing on docs/API.md but they are supposed to be like that as during the repo creation we assume the defaut values and they can be changed on PUT request
I have considered this, see my previous feedback in your previous comments, but as soon as this repo is created, it is enabled, means user are able to use it, but lack of gpgkey may result the usage fail. So I think the right solution is: 1. disable temporarily un-usable repo 2. enable a usable repo
repo = {'baseurl': baseurl, 'mirrorlist': mirrorlist, - 'name': repo_name, 'gpgcheck': 1, - 'gpgkey': [], 'enabled': 1} + 'name': repo_name, 'gpgcheck': gpgcheck, + 'gpgkey': gpgkey, 'enabled': 1}
# write a repo file in the system with repo{} information. parser = ConfigParser() @@ -318,6 +324,7 @@ class AptRepo(object): """ TYPE = 'deb' KIMCHI_LIST = "kimchi-source.list" + CONFIG_ENTRY = ('dist', 'comps')
def __init__(self): getattr(__import__('apt_pkg'), 'init_config')() @@ -413,12 +420,15 @@ class AptRepo(object): # To create a APT repository the dist is a required parameter # (in addition to baseurl, verified on controller through API.json) config = params.get('config', None) + extra_keys = list(set(config.keys()).difference(set(self.CONFIG_ENTRY))) + if len(extra_keys) > 0: + raise InvalidParameter("KCHREPOS0028E", + {'items': ",".join(extra_keys)}) if config is None: raise MissingParameter("KCHREPOS0019E")
if 'dist' not in config.keys(): raise MissingParameter("KCHREPOS0019E") - uri = params['baseurl'] dist = config['dist'] comps = config.get('comps', []) diff --git a/tests/test_model.py b/tests/test_model.py index ceedc6f..56a8e12 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -1295,11 +1295,21 @@ class ModelTests(unittest.TestCase): {'baseurl': 'http://br.archive.kimchi.com/ubuntu/fake', 'config': {'dist': 'quantal', 'comps': ['main']}}]
+ invalid_deb_repo = {'baseurl': 'http://br.archive.ubuntu.com/ubuntu/', + 'config': {'gpgkey': 'file:///fake-key-file'}} + + invalid_yum_repo = { + 'repo_id': 'fedora-fake2', + 'baseurl': 'http://www.fedora.org', + 'config': {'dist': 'quantal', 'comps': ['main']}} + repo_type = inst.capabilities_lookup()['repo_mngt_tool'] if repo_type == 'yum': test_repos = yum_repos + invalid_repo = invalid_deb_repo elif repo_type == 'deb': test_repos = deb_repos + invalid_repo = invalid_yum_repo else: # repository management tool was not recognized by Kimchi # skip test case @@ -1317,11 +1327,15 @@ class ModelTests(unittest.TestCase): 'config': {'dist': 'quantal'}} self.assertRaises(InvalidParameter, inst.repositories_create, repo)
+ # Create repo with invalid config + self.assertRaises(InvalidParameter, inst.repositories_create, repo) + # create repositories with invalid mirrorlist - for url in invalid_urls: - repo = {'repo_id': 'repo_fake', - 'config': {'mirrorlist': url, 'dist': 'quantal'}} - self.assertRaises(InvalidParameter, inst.repositories_create, repo)
+ if repo_type == 'yum': + for url in invalid_urls: + repo = {'repo_id': 'repo_fake', + 'config': {'mirrorlist': url, 'dist': 'quantal'}} + self.assertRaises(InvalidParameter, inst.repositories_create, repo)
You are restricting the test for yum repos. You should create the repo params according to repo type to test deb and yum
for url in invalid_urls: if repo_type == 'yum': repo = {} elif repo_type == 'deb': repo = {}
self.assertRaises(...)
for repo in test_repos: system_host_repos = len(inst.repositories_get_list())
From: Royce Lv <lvroyce@linux.vnet.ibm.com> When using apt, the base url needs to be validated, a fake url will be rejected by kimchi, causing: Traceback (most recent call last): File "../kimchi/tests/test_model.py", line 1443, in test_repository_disable_enable repo_id = inst.repositories_create(repo) File "../kimchi/src/kimchi/model/host.py", line 406, in create return self.host_repositories.addRepository(params) File "../kimchi/src/kimchi/repositories.py", line 54, in addRepository return self._pkg_mnger.addRepo(params) File "../kimchi/src/kimchi/repositories.py", line 426, in addRepo validate_repo_url(uri) File "../kimchi/src/kimchi/utils.py", line 307, in validate_repo_url raise InvalidParameter("KCHUTILS0001E", {'uri': url}) InvalidParameter: KCHUTILS0001E: Invalid URI http://br.archive.ubuntu.com/kimchi/fake So give a valid url in testcase instead of a fake one. Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- tests/test_model.py | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/tests/test_model.py b/tests/test_model.py index 56a8e12..4e517c1 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -1290,9 +1290,9 @@ class ModelTests(unittest.TestCase): {'mirrorlist': 'http://www.fedoraproject.org', 'gpgkey': 'file:///tmp/KEY-fedora-updates-fake-19'}}] - deb_repos = [{'baseurl': 'http://br.archive.ubuntu.com/kimchi/fake', + deb_repos = [{'baseurl': 'http://br.archive.ubuntu.com/ubuntu/', 'config': {'dist': 'quantal'}}, - {'baseurl': 'http://br.archive.kimchi.com/ubuntu/fake', + {'baseurl': 'http://br.archive.ubuntu.com/ubuntu/', 'config': {'dist': 'quantal', 'comps': ['main']}}] invalid_deb_repo = {'baseurl': 'http://br.archive.ubuntu.com/ubuntu/', @@ -1374,9 +1374,9 @@ class ModelTests(unittest.TestCase): 'baseurl': 'http://www.fedora.org'} yum_new_repo = {'baseurl': 'http://www.fedoraproject.org'} - deb_repo = {'baseurl': 'http://br.archive.ubuntu.com/kimchi/fake', + deb_repo = {'baseurl': 'http://br.archive.ubuntu.com/ubuntu/', 'config': {'dist': 'quantal'}} - deb_new_repo = {'baseurl': 'http://archive.canonical.com/kimchi'} + deb_new_repo = {'baseurl': 'http://archive.canonical.com/dists/'} repo_type = inst.capabilities_lookup()['repo_mngt_tool'] if repo_type == 'yum': @@ -1408,10 +1408,11 @@ class ModelTests(unittest.TestCase): repo_id, wrong_repo) # update repositories with invalid mirrorlist - for url in invalid_urls: - wrong_repo = {'config': {'mirrorlist': url}} - self.assertRaises(InvalidParameter, inst.repository_update, - repo_id, wrong_repo) + if repo_type == 'yum': + for url in invalid_urls: + wrong_repo = {'config': {'mirrorlist': url}} + self.assertRaises(InvalidParameter, inst.repository_update, + repo_id, wrong_repo) new_repo_id = inst.repository_update(repo_id, new_repo) repo_info = inst.repository_lookup(new_repo_id) @@ -1429,7 +1430,7 @@ class ModelTests(unittest.TestCase): yum_repo = {'repo_id': 'fedora-fake', 'baseurl': 'http://www.fedora.org'} - deb_repo = {'baseurl': 'http://br.archive.ubuntu.com/kimchi/fake', + deb_repo = {'baseurl': 'http://br.archive.ubuntu.com/ubuntu/', 'config': {'dist': 'quantal'}} repo_type = inst.capabilities_lookup()['repo_mngt_tool'] -- 1.8.3.2
On 09/22/2014 06:32 AM, lvroyce@linux.vnet.ibm.com wrote:
From: Royce Lv <lvroyce@linux.vnet.ibm.com>
When using apt, the base url needs to be validated, a fake url will be rejected by kimchi, causing:
Traceback (most recent call last): File "../kimchi/tests/test_model.py", line 1443, in test_repository_disable_enable repo_id = inst.repositories_create(repo) File "../kimchi/src/kimchi/model/host.py", line 406, in create return self.host_repositories.addRepository(params) File "../kimchi/src/kimchi/repositories.py", line 54, in addRepository return self._pkg_mnger.addRepo(params) File "../kimchi/src/kimchi/repositories.py", line 426, in addRepo validate_repo_url(uri) File "../kimchi/src/kimchi/utils.py", line 307, in validate_repo_url raise InvalidParameter("KCHUTILS0001E", {'uri': url}) InvalidParameter: KCHUTILS0001E: Invalid URI http://br.archive.ubuntu.com/kimchi/fake
So give a valid url in testcase instead of a fake one.
Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- tests/test_model.py | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/tests/test_model.py b/tests/test_model.py index 56a8e12..4e517c1 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -1290,9 +1290,9 @@ class ModelTests(unittest.TestCase): {'mirrorlist': 'http://www.fedoraproject.org', 'gpgkey': 'file:///tmp/KEY-fedora-updates-fake-19'}}]
- deb_repos = [{'baseurl': 'http://br.archive.ubuntu.com/kimchi/fake', + deb_repos = [{'baseurl': 'http://br.archive.ubuntu.com/ubuntu/', 'config': {'dist': 'quantal'}}, - {'baseurl': 'http://br.archive.kimchi.com/ubuntu/fake', + {'baseurl': 'http://br.archive.ubuntu.com/ubuntu/', 'config': {'dist': 'quantal', 'comps': ['main']}}]
invalid_deb_repo = {'baseurl': 'http://br.archive.ubuntu.com/ubuntu/', @@ -1374,9 +1374,9 @@ class ModelTests(unittest.TestCase): 'baseurl': 'http://www.fedora.org'} yum_new_repo = {'baseurl': 'http://www.fedoraproject.org'}
- deb_repo = {'baseurl': 'http://br.archive.ubuntu.com/kimchi/fake', + deb_repo = {'baseurl': 'http://br.archive.ubuntu.com/ubuntu/', 'config': {'dist': 'quantal'}} - deb_new_repo = {'baseurl': 'http://archive.canonical.com/kimchi'} + deb_new_repo = {'baseurl': 'http://archive.canonical.com/dists/'}
repo_type = inst.capabilities_lookup()['repo_mngt_tool'] if repo_type == 'yum': @@ -1408,10 +1408,11 @@ class ModelTests(unittest.TestCase): repo_id, wrong_repo)
# update repositories with invalid mirrorlist - for url in invalid_urls: - wrong_repo = {'config': {'mirrorlist': url}} - self.assertRaises(InvalidParameter, inst.repository_update, - repo_id, wrong_repo)
+ if repo_type == 'yum': + for url in invalid_urls: + wrong_repo = {'config': {'mirrorlist': url}} + self.assertRaises(InvalidParameter, inst.repository_update, + repo_id, wrong_repo)
Same I commented on previous patch. You are restricting the test for yum repos.
new_repo_id = inst.repository_update(repo_id, new_repo) repo_info = inst.repository_lookup(new_repo_id) @@ -1429,7 +1430,7 @@ class ModelTests(unittest.TestCase):
yum_repo = {'repo_id': 'fedora-fake', 'baseurl': 'http://www.fedora.org'} - deb_repo = {'baseurl': 'http://br.archive.ubuntu.com/kimchi/fake', + deb_repo = {'baseurl': 'http://br.archive.ubuntu.com/ubuntu/', 'config': {'dist': 'quantal'}}
repo_type = inst.capabilities_lookup()['repo_mngt_tool']
From: Royce Lv <lvroyce@linux.vnet.ibm.com> When update repository, we remove a old repository first then add a new one, if error occurs when adding new one, old one was removed which will cause problem. Fix by recovering removed old repository after failure of adding new one. Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/repositories.py | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/src/kimchi/repositories.py b/src/kimchi/repositories.py index 3ac0e9c..9d665d5 100644 --- a/src/kimchi/repositories.py +++ b/src/kimchi/repositories.py @@ -17,6 +17,7 @@ # License along with this library; if not, write to the Free Software # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA +import copy import os import time import urlparse @@ -29,7 +30,6 @@ from kimchi.exception import InvalidOperation, InvalidParameter from kimchi.exception import OperationFailed, NotFoundError, MissingParameter from kimchi.utils import validate_repo_url - class Repositories(object): __metaclass__ = Singleton @@ -446,7 +446,6 @@ class AptRepo(object): kimchiLock.release() raise OperationFailed("KCHREPOS0026E", {'err': e.message}) kimchiLock.release() - return self._get_repo_id(source_entry) def toggleRepo(self, repo_id, enable): @@ -490,22 +489,27 @@ class AptRepo(object): """ Update a given repository in repositories.Repositories() format """ - r = self._get_source_entry(repo_id) - if r is None: - raise NotFoundError("KCHREPOS0012E", {'repo_id': repo_id}) - - info = {'enabled': not r.disabled, - 'baseurl': params.get('baseurl', r.uri), - 'config': {'type': 'deb', 'dist': r.dist, - 'comps': r.comps}} + old_info = self.getRepo(repo_id) + updated_info = copy.deepcopy(old_info) + updated_info['baseurl'] = params.get('baseurl', updated_info['baseurl']) if 'config' in params.keys(): config = params['config'] - info['config']['dist'] = config.get('dist', r.dist) - info['config']['comps'] = config.get('comps', r.comps) + updated_info['config']['dist'] = config.get('dist', r.dist) + updated_info['config']['comps'] = config.get('comps', r.comps) - self.removeRepo(repo_id) - return self.addRepo(info) + try: + new_id = None + self.removeRepo(repo_id) + removed = True + new_id = self.addRepo(updated_info) + return new_id + except: + if removed: + self.addRepo(old_info) + if new_id: + self.removeRepo(new_id) + raise def removeRepo(self, repo_id): """ -- 1.8.3.2
On 09/22/2014 06:32 AM, lvroyce@linux.vnet.ibm.com wrote:
From: Royce Lv <lvroyce@linux.vnet.ibm.com>
When update repository, we remove a old repository first then add a new one, if error occurs when adding new one, old one was removed which will cause problem.
Fix by recovering removed old repository after failure of adding new one.
Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/repositories.py | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-)
diff --git a/src/kimchi/repositories.py b/src/kimchi/repositories.py index 3ac0e9c..9d665d5 100644 --- a/src/kimchi/repositories.py +++ b/src/kimchi/repositories.py @@ -17,6 +17,7 @@ # License along with this library; if not, write to the Free Software # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+import copy import os import time import urlparse @@ -29,7 +30,6 @@ from kimchi.exception import InvalidOperation, InvalidParameter from kimchi.exception import OperationFailed, NotFoundError, MissingParameter from kimchi.utils import validate_repo_url
- class Repositories(object): __metaclass__ = Singleton
@@ -446,7 +446,6 @@ class AptRepo(object): kimchiLock.release() raise OperationFailed("KCHREPOS0026E", {'err': e.message}) kimchiLock.release() - return self._get_repo_id(source_entry)
def toggleRepo(self, repo_id, enable): @@ -490,22 +489,27 @@ class AptRepo(object): """ Update a given repository in repositories.Repositories() format """ - r = self._get_source_entry(repo_id) - if r is None: - raise NotFoundError("KCHREPOS0012E", {'repo_id': repo_id}) - - info = {'enabled': not r.disabled, - 'baseurl': params.get('baseurl', r.uri), - 'config': {'type': 'deb', 'dist': r.dist, - 'comps': r.comps}} + old_info = self.getRepo(repo_id) + updated_info = copy.deepcopy(old_info) + updated_info['baseurl'] = params.get('baseurl', updated_info['baseurl'])
if 'config' in params.keys(): config = params['config'] - info['config']['dist'] = config.get('dist', r.dist) - info['config']['comps'] = config.get('comps', r.comps) + updated_info['config']['dist'] = config.get('dist', r.dist) + updated_info['config']['comps'] = config.get('comps', r.comps)
- self.removeRepo(repo_id) - return self.addRepo(info)
+ try: + new_id = None + self.removeRepo(repo_id) + removed = True + new_id = self.addRepo(updated_info) + return new_id
+ except: + if removed: + self.addRepo(old_info)
If an exception occurs on self.removeRepo() the 'removed' variable will be undefined
+ if new_id: + self.removeRepo(new_id)
What is the case to remove the updated repo? if 'new_id' has a value the update occurs with success.
+ raise
What about? # remove repo may not be in the try/except block as we want to raise the error if any occurs self.removeRepo(repo_id) try: self.addRepo(updated_info) except: self.addRepo(old_info) raise OperationFailed(...)
def removeRepo(self, repo_id): """
On 2014年09月22日 23:27, Aline Manera wrote:
On 09/22/2014 06:32 AM, lvroyce@linux.vnet.ibm.com wrote:
From: Royce Lv <lvroyce@linux.vnet.ibm.com>
When update repository, we remove a old repository first then add a new one, if error occurs when adding new one, old one was removed which will cause problem.
Fix by recovering removed old repository after failure of adding new one.
Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/repositories.py | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-)
diff --git a/src/kimchi/repositories.py b/src/kimchi/repositories.py index 3ac0e9c..9d665d5 100644 --- a/src/kimchi/repositories.py +++ b/src/kimchi/repositories.py @@ -17,6 +17,7 @@ # License along with this library; if not, write to the Free Software # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+import copy import os import time import urlparse @@ -29,7 +30,6 @@ from kimchi.exception import InvalidOperation, InvalidParameter from kimchi.exception import OperationFailed, NotFoundError, MissingParameter from kimchi.utils import validate_repo_url
- class Repositories(object): __metaclass__ = Singleton
@@ -446,7 +446,6 @@ class AptRepo(object): kimchiLock.release() raise OperationFailed("KCHREPOS0026E", {'err': e.message}) kimchiLock.release() - return self._get_repo_id(source_entry)
def toggleRepo(self, repo_id, enable): @@ -490,22 +489,27 @@ class AptRepo(object): """ Update a given repository in repositories.Repositories() format """ - r = self._get_source_entry(repo_id) - if r is None: - raise NotFoundError("KCHREPOS0012E", {'repo_id': repo_id}) - - info = {'enabled': not r.disabled, - 'baseurl': params.get('baseurl', r.uri), - 'config': {'type': 'deb', 'dist': r.dist, - 'comps': r.comps}} + old_info = self.getRepo(repo_id) + updated_info = copy.deepcopy(old_info) + updated_info['baseurl'] = params.get('baseurl', updated_info['baseurl'])
if 'config' in params.keys(): config = params['config'] - info['config']['dist'] = config.get('dist', r.dist) - info['config']['comps'] = config.get('comps', r.comps) + updated_info['config']['dist'] = config.get('dist', r.dist) + updated_info['config']['comps'] = config.get('comps', r.comps)
- self.removeRepo(repo_id) - return self.addRepo(info)
+ try: + new_id = None + self.removeRepo(repo_id) + removed = True + new_id = self.addRepo(updated_info) + return new_id
+ except: + if removed: + self.addRepo(old_info)
If an exception occurs on self.removeRepo() the 'removed' variable will be undefined
+ if new_id: + self.removeRepo(new_id)
What is the case to remove the updated repo? if 'new_id' has a value the update occurs with success.
+ raise
What about?
# remove repo may not be in the try/except block as we want to raise the error if any occurs self.removeRepo(repo_id)
try: self.addRepo(updated_info) except: self.addRepo(old_info) raise OperationFailed(...)
Good point
def removeRepo(self, repo_id): """
participants (3)
-
Aline Manera -
lvroyce＠linux.vnet.ibm.com -
Royce Lv