From 8c64920d814422d074aaf296ef57e71bfe4c47a9 Mon Sep 17 00:00:00 2001 From: Filip Valder Date: Thu, 10 Nov 2016 12:00:28 +0100 Subject: [PATCH 1/9] ConfigParser module added --- requirements.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/requirements.txt b/requirements.txt index feac1a8e..9fd2f9ad 100644 --- a/requirements.txt +++ b/requirements.txt @@ -14,3 +14,4 @@ python-fedora funcsigs # Python2 only m2crypto m2ext +ConfigParser From e783e91889f01286380a2ac12c792f71f1f7c4c5 Mon Sep 17 00:00:00 2001 From: Filip Valder Date: Thu, 10 Nov 2016 12:00:51 +0100 Subject: [PATCH 2/9] sort by name --- requirements.txt | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/requirements.txt b/requirements.txt index 9fd2f9ad..9458600a 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,17 +1,17 @@ -flask -sqlalchemy -six -fedmsg -pdc-client -modulemd==0.1 -pyOpenSSL -kobo -munch -Flask-Script -Flask-SQLAlchemy +ConfigParser Flask-Migrate -python-fedora +Flask-SQLAlchemy +Flask-Script +fedmsg +flask funcsigs # Python2 only +kobo m2crypto m2ext -ConfigParser +modulemd==0.1 +munch +pdc-client +pyOpenSSL +python-fedora +six +sqlalchemy From b9382f8e9f248fb591b6b9cdccf6ccded567a743 Mon Sep 17 00:00:00 2001 From: Filip Valder Date: Fri, 11 Nov 2016 14:05:57 +0100 Subject: [PATCH 3/9] refactor: registering conf items is not required --- module_build_service/config.py | 606 +++++++++++++-------------------- 1 file changed, 228 insertions(+), 378 deletions(-) diff --git a/module_build_service/config.py b/module_build_service/config.py index 904f8f3d..d27b1fd8 100644 --- a/module_build_service/config.py +++ b/module_build_service/config.py @@ -30,13 +30,174 @@ import six from module_build_service import app from module_build_service import logger +DEFAULTS = [ + {'name': 'system', + 'type': str, + 'default': '', + 'desc': ''}, + {'name': 'db', + 'type': str, + 'default': '', + 'desc': ''}, + {'name': 'polling_interval', + 'type': int, + 'default': 0, + 'desc': ''}, + {'name': 'pdc_url', + 'type': str, + 'default': '', + 'desc': ''}, + {'name': 'pdc_insecure', + 'type': bool, + 'default': False, + 'desc': ''}, + {'name': 'pdc_develop', + 'type': bool, + 'default': False, + 'desc': ''}, + {'name': 'koji_config', + 'type': str, + 'default': None, + 'desc': ''}, + {'name': 'koji_profile', + 'type': str, + 'default': None, + 'desc': ''}, + {'name': 'koji_arches', + 'type': list, + 'default': None, + 'desc': ''}, + {'name': 'koji_proxyuser', + 'type': bool, + 'default': None, + 'desc': ''}, + {'name': 'koji_build_priority', + 'type': int, + 'default': 10, + 'desc': ''}, + {'name': 'koji_repository_url', + 'type': str, + 'default': None, + 'desc': ''}, + {'name': 'rpms_default_repository', + 'type': str, + 'default': '', + 'desc': ''}, + {'name': 'rpms_allow_repository', + 'type': bool, + 'default': False, + 'desc': ''}, + {'name': 'rpms_default_cache', + 'type': str, + 'default': '', + 'desc': ''}, + {'name': 'rpms_allow_cache', + 'type': bool, + 'default': False, + 'desc': ''}, + {'name': 'ssl_certificate_file', + 'type': str, + 'default': '', + 'desc': ''}, + {'name': 'ssl_certificate_key_file', + 'type': str, + 'default': '', + 'desc': ''}, + {'name': 'ssl_ca_certificate_file', + 'type': str, + 'default': '', + 'desc': ''}, + {'name': 'pkgdb_api_url', + 'type': str, + 'default': '', + 'desc': ''}, + {'name': 'fas_url', + 'type': str, + 'default': '', + 'desc': ''}, + {'name': 'fas_username', + 'type': str, + 'default': '', + 'desc': ''}, + {'name': 'fas_password', + 'type': str, + 'default': '', + 'desc': ''}, + {'name': 'require_packager', + 'type': bool, + 'default': True, + 'desc': ''}, + {'name': 'log_backend', + 'type': str, + 'default': '', + 'desc': ''}, + {'name': 'log_file', + 'type': str, + 'default': '', + 'desc': ''}, + {'name': 'log_level', + 'type': str, + 'default': 0, + 'desc': ''}, + {'name': 'krb_keytab', + 'type': None, + 'default': None, + 'desc': ''}, + {'name': 'krb_principal', + 'type': None, + 'default': None, + 'desc': ''}, + {'name': 'krb_ccache', + 'type': None, + 'default': '/tmp/krb5cc_module_build_service', + 'desc': ''}, + {'name': 'messaging', + 'type': str, + 'default': '', + 'desc': ''}, + {'name': 'amq_recv_addresses', + 'type': list, + 'default': [], + 'desc': ''}, + {'name': 'amq_dest_address', + 'type': str, + 'default': '', + 'desc': ''}, + {'name': 'amq_cert_file', + 'type': str, + 'default': '', + 'desc': ''}, + {'name': 'amq_private_key_file', + 'type': str, + 'default': '', + 'desc': ''}, + {'name': 'amq_trusted_cert_file', + 'type': str, + 'default': '', + 'desc': ''}, + {'name': 'mock_config', + 'type': str, + 'default': 'fedora-25-x86_64', + 'desc': ''}, + {'name': 'mock_build_srpm_cmd', + 'type': str, + 'default': 'fedpkg --dist f25 srpm', + 'desc': ''}, + {'name': 'scmurls', + 'type': list, + 'default': [], + 'desc': ''}, +] + def from_app_config(): """ Create the configuration instance from the values in app.config """ conf = Config() for key, value in app.config.items(): - setattr(conf, key.lower(), value) + # lower keys + key = key.lower() + conf.setitem(key, value) return conf @@ -44,407 +205,96 @@ class Config(object): """Class representing the orchestrator configuration.""" def __init__(self): - """Initialize the Config object.""" - self._system = "" - self._db = "" - self._polling_interval = 0 - self._pdc_url = "" - self._pdc_insecure = False - self._pdc_develop = False - self._koji_config = None - self._koji_profile = None - self._koji_arches = None - self._koji_proxyuser = None - self._koji_build_priority = 10 - self._koji_repository_url = None - self._rpms_default_repository = "" - self._rpms_allow_repository = False - self._rpms_default_cache = "" - self._rpms_allow_cache = False - self._ssl_certificate_file = "" - self._ssl_certificate_key_file = "" - self._ssl_ca_certificate_file = "" - self._pkgdb_api_url = "" - self._fas_url = "" - self._fas_username = "" - self._fas_password = "" - self._require_packager = True - self._log_backend = "" - self._log_file = "" - self._log_level = 0 - self._krb_keytab = None - self._krb_principal = None - self._krb_ccache = "/tmp/krb5cc_module_build_service" - self._messaging = "" - self._amq_recv_addresses = [] - self._amq_dest_address = "" - self._amq_cert_file = "" - self._amq_private_key_file = "" - self._amq_trusted_cert_file = "" - self._mock_config = "fedora-25-x86_64" - self._mock_build_srpm_cmd = "fedpkg --dist f25 srpm" + """Initialize the Config object with defaults.""" + self._defaults = DEFAULTS + self._defaults_by_name = {conf_item['name']: conf_item + for conf_item + in self._defaults} - @property - def system(self): - """The buildsystem to use.""" - return self._system + for conf_item in self._defaults: + self.setitem(conf_item['name'], conf_item['default']) - @system.setter - def system(self, s): + def setitem(self, key, value): + if key == 'setitem' or key.startswith('_'): + raise Exception("Configuration item's name is not allowed: %s" % key) + + # registered defaults + if key in self._defaults_by_name: + # customized check & set first + setifok_func = '_setifok_{}'.format(key) + if hasattr(self, setifok_func): + getattr(self, setifok_func)(value) + return + + # type conversion + convert = self._defaults_by_name[key]['type'] + if convert in [bool, int, list, str]: + setattr(self, key, convert(value)) + # conversion is not required if type is None + elif convert is None: + setattr(self, key, value) + # unknown type/unsupported conversion + else: + raise Exception("Unsupported type %s for configuration item: %s" % (convert, key)) + # passthrough for uncontrolled configuration items + else: + setattr(self, key, value) + + return + + def _setifok_system(self, s): s = str(s) if s not in ("koji", "copr", "mock"): raise ValueError("Unsupported buildsystem: %s." % s) - self._system = s + self.system = s - @property - def messaging(self): - """The messaging system to use.""" - return self._messaging - - @messaging.setter - def messaging(self, s): - s = str(s) - if s not in ("fedmsg" , "amq"): - raise ValueError("Unsupported messaging system.") - self._messaging = s - - @property - def amq_recv_addresses(self): - """Apache MQ broker url to receive messages.""" - return self._amq_recv_addresses - - @amq_recv_addresses.setter - def amq_recv_addresses(self, l): - assert isinstance(l, list) or isinstance(l, tuple) - self._amq_recv_addresses = list(l) - - @property - def amq_dest_address(self): - """Apache MQ broker address to send messages""" - return self._amq_dest_address - - @amq_dest_address.setter - def amq_dest_address(self, s): - self._amq_dest_address = str(s) - - @property - def amq_cert_file(self): - """Certificate for Apache MQ broker auth.""" - return self._amq_cert_file - - @amq_cert_file.setter - def amq_cert_file(self, s): - self._amq_cert_file = str(s) - - @property - def amq_private_key_file(self): - """Private key for Apache MQ broker auth.""" - return self._amq_private_key_file - - @amq_private_key_file.setter - def amq_private_key_file(self, s): - self._amq_private_key_file = str(s) - - @property - def amq_trusted_cert_file(self): - """Trusted certificate for ssl connection.""" - return self._amq_trusted_cert_file - - @amq_trusted_cert_file.setter - def amq_trusted_cert_file(self, s): - s = str(s) - self._amq_trusted_cert_file = s - - - @property - def db(self): - """RDB URL.""" - return self._db - - @db.setter - def db(self, s): - self._db = str(s) - - @property - def pdc_url(self): - """PDC URL.""" - return self._pdc_url - - @pdc_url.setter - def pdc_url(self, s): - self._pdc_url = str(s) - - @property - def pdc_insecure(self): - """Allow insecure connection to PDC.""" - return self._pdc_insecure - - @pdc_insecure.setter - def pdc_insecure(self, b): - self._pdc_insecure = bool(b) - - @property - def pdc_develop(self): - """PDC Development mode, basically noauth.""" - return self._pdc_develop - - @pdc_develop.setter - def pdc_develop(self, b): - self._pdc_develop = bool(b) - - @property - def polling_interval(self): - """Polling interval, in seconds.""" - return self._polling_interval - - @polling_interval.setter - def polling_interval(self, i): + def _setifok_polling_interval(self, i): if not isinstance(i, int): raise TypeError("polling_interval needs to be an int") if i < 0: raise ValueError("polling_interval must be >= 0") - self._polling_interval = i + self.polling_interval = i - @property - def mock_config(self): - return self._mock_config - - @mock_config.setter - def mock_config(self, s): - self._mock_config = str(s) - - @property - def mock_build_srpm_cmd(self): - return self._mock_build_srpm_cmd - - @mock_build_srpm_cmd.setter - def mock_build_srpm_cmd(self, s): - self._mock_build_srpm_cmd = str(s) - - @property - def koji_config(self): - """Koji URL.""" - return self._koji_config - - @koji_config.setter - def koji_config(self, s): - self._koji_config = str(s) - - @property - def koji_profile(self): - """Koji URL.""" - return self._koji_profile - - @koji_profile.setter - def koji_profile(self, s): - self._koji_profile = str(s) - - @property - def koji_arches(self): - """Koji architectures.""" - return self._koji_arches - - @koji_arches.setter - def koji_arches(self, s): - self._koji_arches = list(s) - - @property - def koji_proxyuser(self): - """Koji proxyuser flag.""" - return self._koji_proxyuser - - @koji_proxyuser.setter - def koji_proxyuser(self, s): - self._koji_proxyuser = bool(s) - - @property - def koji_repository_url(self): - return self._koji_repository_url - - @koji_repository_url.setter - def koji_repository_url(self, s): - self._koji_repository_url = str(s) - - @property - def koji_build_priority(self): - return self._koji_build_priority - - @koji_build_priority.setter - def koji_build_priority(self, i): - if not isinstance(i, int): - raise TypeError("koji_build_priority needs to be an int") - self._koji_build_priority = i - - @property - def scmurls(self): - """Allowed SCM URLs.""" - return self._scmurls - - @scmurls.setter - def scmurls(self, l): - if not isinstance(l, list): - raise TypeError("scmurls needs to be a list.") - self._scmurls = [str(x) for x in l] - - @property - def rpms_default_repository(self): - return self._rpms_default_repository - - @rpms_default_repository.setter - def rpms_default_repository(self, s): + def _setifok_rpms_default_repository(self, s): rpm_repo = str(s) if rpm_repo[-1] != '/': rpm_repo = rpm_repo + '/' - self._rpms_default_repository = rpm_repo + self.rpms_default_repository = rpm_repo - @property - def rpms_allow_repository(self): - return self._rpms_allow_repository - - @rpms_allow_repository.setter - def rpms_allow_repository(self, b): - if not isinstance(b, bool): - raise TypeError("rpms_allow_repository must be a bool.") - self._rpms_allow_repository = b - - @property - def rpms_default_cache(self): - return self._rpms_default_cache - - @rpms_default_cache.setter - def rpms_default_cache(self, s): + def _setifok_rpms_default_cache(self, s): rpm_cache = str(s) if rpm_cache[-1] != '/': rpm_cache = rpm_cache + '/' - self._rpms_default_cache = rpm_cache + self.rpms_default_cache = rpm_cache - @property - def rpms_allow_cache(self): - return self._rpms_allow_cache - - @rpms_allow_cache.setter - def rpms_allow_cache(self, b): - if not isinstance(b, bool): - raise TypeError("rpms_allow_cache must be a bool.") - self._rpms_allow_cache = b - - @property - def ssl_certificate_file(self): - return self._ssl_certificate_file - - @ssl_certificate_file.setter - def ssl_certificate_file(self, s): - self._ssl_certificate_file = str(s) - - @property - def ssl_ca_certificate_file(self): - return self._ssl_ca_certificate_file - - @ssl_ca_certificate_file.setter - def ssl_ca_certificate_file(self, s): - self._ssl_ca_certificate_file = str(s) - - @property - def ssl_certificate_key_file(self): - return self._ssl_certificate_key_file - - @ssl_certificate_key_file.setter - def ssl_certificate_key_file(self, s): - self._ssl_certificate_key_file = str(s) - - @property - def pkgdb_api_url(self): - return self._pkgdb_api_url - - @pkgdb_api_url.setter - def pkgdb_api_url(self, s): - self._pkgdb_api_url = str(s) - - @property - def fas_url(self): - return self._fas_url - - @fas_url.setter - def fas_url(self, s): - self._fas_url = str(s) - - @property - def fas_username(self): - return self._fas_username - - @fas_username.setter - def fas_username(self, s): - self._fas_username = str(s) - - @property - def fas_password(self): - return self._fas_password - - @fas_password.setter - def fas_password(self, s): - self._fas_password = str(s) - - @property - def require_packager(self): - return self._require_packager - - @require_packager.setter - def require_packager(self, s): - self._require_packager = bool(s) - - @property - def log_backend(self): - return self._log_backend - - @log_backend.setter - def log_backend(self, s): - if s == None: - self._log_backend = "console" - elif not s in logger.supported_log_backends(): + def _setifok_log_backend(self, s): + if s is None: + self.log_backend = "console" + elif s not in logger.supported_log_backends(): raise ValueError("Unsupported log backend") + self.log_backend = str(s) - self._log_backend = str(s) - - @property - def log_file(self): - return self._log_file - - @log_file.setter - def log_file(self, s): - if s == None: - self._log_file = "" + def _setifok_log_file(self, s): + if s is None: + self.log_file = "" else: - self._log_file = str(s) + self.log_file = str(s) - @property - def log_level(self): - return self._log_level - - @log_level.setter - def log_level(self, s): + def _setifok_log_level(self, s): level = str(s).lower() - self._log_level = logger.str_to_log_level(level) + self.log_level = logger.str_to_log_level(level) - @property - def krb_keytab(self): - return self._krb_keytab + def _setifok_messaging(self, s): + s = str(s) + if s not in ("fedmsg", "amq"): + raise ValueError("Unsupported messaging system.") + self.messaging = s - @krb_keytab.setter - def krb_keytab(self, s): - self._krb_keytab = s + def _setifok_amq_recv_addresses(self, l): + assert isinstance(l, list) or isinstance(l, tuple) + self.amq_recv_addresses = list(l) - @property - def krb_principal(self): - return self._krb_principal - - @krb_principal.setter - def krb_principal(self, s): - self._krb_principal = s - - @property - def krb_ccache(self): - return self._krb_ccache - - @krb_ccache.setter - def krb_ccache(self, s): - self._krb_ccache = s + def _setifok_scmurls(self, l): + if not isinstance(l, list): + raise TypeError("scmurls needs to be a list.") + self.scmurls = [str(x) for x in l] From 5c3a46c194d33d0721716cd299b764e9b9149fbc Mon Sep 17 00:00:00 2001 From: Filip Valder Date: Fri, 11 Nov 2016 14:22:32 +0100 Subject: [PATCH 4/9] fix: reasonable defaults or taken from runtime configuration (top-level ocnfig.py) --- module_build_service/config.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/module_build_service/config.py b/module_build_service/config.py index d27b1fd8..8d2faf67 100644 --- a/module_build_service/config.py +++ b/module_build_service/config.py @@ -33,7 +33,7 @@ from module_build_service import logger DEFAULTS = [ {'name': 'system', 'type': str, - 'default': '', + 'default': 'koji', 'desc': ''}, {'name': 'db', 'type': str, @@ -65,7 +65,7 @@ DEFAULTS = [ 'desc': ''}, {'name': 'koji_arches', 'type': list, - 'default': None, + 'default': [], 'desc': ''}, {'name': 'koji_proxyuser', 'type': bool, @@ -81,7 +81,7 @@ DEFAULTS = [ 'desc': ''}, {'name': 'rpms_default_repository', 'type': str, - 'default': '', + 'default': 'git://pkgs.fedoraproject.org/rpms/', 'desc': ''}, {'name': 'rpms_allow_repository', 'type': bool, @@ -89,7 +89,7 @@ DEFAULTS = [ 'desc': ''}, {'name': 'rpms_default_cache', 'type': str, - 'default': '', + 'default': 'http://pkgs.fedoraproject.org/repo/pkgs/', 'desc': ''}, {'name': 'rpms_allow_cache', 'type': bool, @@ -129,7 +129,7 @@ DEFAULTS = [ 'desc': ''}, {'name': 'log_backend', 'type': str, - 'default': '', + 'default': None, 'desc': ''}, {'name': 'log_file', 'type': str, @@ -153,7 +153,7 @@ DEFAULTS = [ 'desc': ''}, {'name': 'messaging', 'type': str, - 'default': '', + 'default': 'fedmsg', 'desc': ''}, {'name': 'amq_recv_addresses', 'type': list, From 6a026e4d5b9d8f1d1f42a53b16634dd124af9596 Mon Sep 17 00:00:00 2001 From: Filip Valder Date: Fri, 11 Nov 2016 14:24:55 +0100 Subject: [PATCH 5/9] fix: helps identifying conf var name instead of generic TypeError --- module_build_service/config.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/module_build_service/config.py b/module_build_service/config.py index 8d2faf67..3a5f86ce 100644 --- a/module_build_service/config.py +++ b/module_build_service/config.py @@ -229,7 +229,10 @@ class Config(object): # type conversion convert = self._defaults_by_name[key]['type'] if convert in [bool, int, list, str]: - setattr(self, key, convert(value)) + try: + setattr(self, key, convert(value)) + except: + raise Exception("Configuration value conversion failed for name: %s" % key) # conversion is not required if type is None elif convert is None: setattr(self, key, value) From 6d0903c872e11709ce7a9da48d508f97de1fe00f Mon Sep 17 00:00:00 2001 From: Filip Valder Date: Fri, 11 Nov 2016 14:28:06 +0100 Subject: [PATCH 6/9] revert: maybe later.. --- requirements.txt | 1 - 1 file changed, 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 9458600a..8c243cf5 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,4 +1,3 @@ -ConfigParser Flask-Migrate Flask-SQLAlchemy Flask-Script From 3313cbb007d6327f2e99fb523e6b08fd613a04f4 Mon Sep 17 00:00:00 2001 From: Filip Valder Date: Fri, 11 Nov 2016 14:53:43 +0100 Subject: [PATCH 7/9] convention: avoid confusion with __setitem__ --- module_build_service/config.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/module_build_service/config.py b/module_build_service/config.py index 3a5f86ce..99206466 100644 --- a/module_build_service/config.py +++ b/module_build_service/config.py @@ -197,7 +197,7 @@ def from_app_config(): for key, value in app.config.items(): # lower keys key = key.lower() - conf.setitem(key, value) + conf.set_item(key, value) return conf @@ -212,10 +212,10 @@ class Config(object): in self._defaults} for conf_item in self._defaults: - self.setitem(conf_item['name'], conf_item['default']) + self.set_item(conf_item['name'], conf_item['default']) - def setitem(self, key, value): - if key == 'setitem' or key.startswith('_'): + def set_item(self, key, value): + if key == 'set_item' or key.startswith('_'): raise Exception("Configuration item's name is not allowed: %s" % key) # registered defaults From 051cbb1d0f9fc9547fd2a9d1533c32dd04bff10c Mon Sep 17 00:00:00 2001 From: Filip Valder Date: Fri, 11 Nov 2016 14:54:13 +0100 Subject: [PATCH 8/9] fix: to be more specific --- module_build_service/config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/module_build_service/config.py b/module_build_service/config.py index 99206466..33850ce7 100644 --- a/module_build_service/config.py +++ b/module_build_service/config.py @@ -238,7 +238,7 @@ class Config(object): setattr(self, key, value) # unknown type/unsupported conversion else: - raise Exception("Unsupported type %s for configuration item: %s" % (convert, key)) + raise Exception("Unsupported type %s for configuration item name: %s" % (convert, key)) # passthrough for uncontrolled configuration items else: setattr(self, key, value) From ea99293919265c0f667582ef96b6e85f47a2700c Mon Sep 17 00:00:00 2001 From: Filip Valder Date: Fri, 11 Nov 2016 16:04:57 +0100 Subject: [PATCH 9/9] fix: conf variable needn't be registered in DEFAULTS to be able to use _setifok_ handler --- module_build_service/config.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/module_build_service/config.py b/module_build_service/config.py index 33850ce7..17a85f3b 100644 --- a/module_build_service/config.py +++ b/module_build_service/config.py @@ -220,7 +220,7 @@ class Config(object): # registered defaults if key in self._defaults_by_name: - # customized check & set first + # customized check & set if there's a corresponding handler setifok_func = '_setifok_{}'.format(key) if hasattr(self, setifok_func): getattr(self, setifok_func)(value) @@ -241,7 +241,13 @@ class Config(object): raise Exception("Unsupported type %s for configuration item name: %s" % (convert, key)) # passthrough for uncontrolled configuration items else: - setattr(self, key, value) + # customized check & set if there's a corresponding handler + setifok_func = '_setifok_{}'.format(key) + if hasattr(self, setifok_func): + getattr(self, setifok_func)(value) + # otherwise just blindly set value for a key + else: + setattr(self, key, value) return