Mtf config file #218

jscotka · 2018-03-02T16:58:28Z

This tries to remove hardcoded variables and move them to config file(s)

TomasTomecek

What is the expected workflow? Will the mtf.yaml config be supplied with the package and users will override it?

TomasTomecek · 2018-03-05T12:18:29Z

moduleframework/mtf.yaml

@@ -0,0 +1,61 @@
+default_module: docker


this file will require a lot of documentation so users would understand what each key means

thanks for these comments.
Yep, I expect that package will provide some default configuration (for testing upstream/fedora)
and some users who understand that, is able to reconfigure it to another value.
from that PoV it is not expected that "normal" user should understand that.

Yep, would be nice to add there documentation. for now probably it is better without documentation, than have these values hardcoded inside python code.

logic of loading files is on lines:
https://github.com/fedora-modularity/meta-test-family/pull/218/files#diff-d49eca679ceb3a9c6a60a7a4afec5f21R89

it will use these files and configuration is updated with each existing file, meand that it loads 1. and in case 3. exist, config is updated by these values

/usr/lib/python2.7/site-packages/moduleframework/mtf.yaml

/etc/mtf.yaml

~/.mtf.yaml

phracek

Sometimes you use common.fce and sometimes only fce. Like print_info.
Also improve ConfigMTF class so that it is able to use getattr method like https://docs.python.org/2/library/functions.html#getattr

phracek · 2018-03-06T14:03:28Z

moduleframework/common.py

@@ -1,4 +1,4 @@
-#
+# -*- coding: utf-8 -*-
 # Meta test family (MTF) is a tool to test components of a modular Fedora:


I would prefer to remove it. It tests also containers. Not or modular Fedora. The line below too.

phracek · 2018-03-06T14:05:24Z

moduleframework/common.py

+
+class MTFConfParser(object):
+    configname = "mtf.yaml"
+    source_cfg_file = os.path.join(os.path.dirname(__file__), configname)


I would move it to /usr/share/modularityframework/. /usr/lib/python2.7/... is for python packages.

I thought about that too and in init version I had it there. I'll move it there

phracek · 2018-03-06T14:06:11Z

moduleframework/common.py

+class MTFConfParser(object):
+    configname = "mtf.yaml"
+    source_cfg_file = os.path.join(os.path.dirname(__file__), configname)
+    default_cfg_file = os.path.join("/etc", configname)


Add /etc at the beginning of the file common.py. There is already set of constants.

phracek · 2018-03-06T14:06:55Z

moduleframework/common.py

+        self.options = config
+
+
+    def get(self, section=None, key=None):


What about to use function getattr like https://docs.python.org/2/library/functions.html#getattr

I'll implement that. sounds well

phracek · 2018-03-06T14:10:03Z

moduleframework/common.py

@@ -868,9 +858,10 @@ def get_module_type():
    """
    amodule = os.environ.get('MODULE')
    readconfig = get_config(fail_without_url=False)
-    if "default_module" in readconfig and readconfig[
-        "default_module"] is not None and amodule is None:
+    if amodule is None and readconfig.get("default_module"):


what about something like:

amodule = getattr(conf[options], "default_module", None)

fixed by elegant way: config parser has dict as parent class, so all dict methods available there.

phracek · 2018-03-06T14:14:40Z

moduleframework/helpers/rpm_helper.py

@@ -104,14 +107,16 @@ def setRepositoriesAndWhatToInstall(self, repos=[], whattooinstall=None):
        if repos:
            self.repos = repos
        else:
-            self.repos += get_compose_url() or self.get_url()
+            self.repos += common.get_compose_url() or self.get_url()


Here you use common., but above only print_info. Any reason?
Keep the python style.

I'll fix it. would be nice also to fix all these issues in next PR, we can do issue for this cleanup

phracek · 2018-03-06T14:15:21Z

moduleframework/mtf.yaml

+
+# openshift specific section
+openshift:
+  init_wait: 50


Add more documentation

I'm not able to add there documenttation, because you wrote this part and I've just moved these constants out of code to this file without knowledge what this does mean

phracek · 2018-03-06T14:16:04Z

moduleframework/mtf.yaml

+   ( test -e /usr/bin/yum && echo 'yum -y'             ||
+   echo 'apt-get -y' ) )"
+# which values are considered as true
+  true_values: ['yes', 'YES', 'yes', 'True', 'true', 'ok', 'OK']


This can be one general function. I don't see any reason, why to have it here, in config file.

phracek · 2018-03-06T14:17:02Z

moduleframework/mtf_scheduler.py

@@ -205,10 +202,10 @@ def __init__(self, args, unknown):
        if args.linter:
            self.tests += glob.glob("{MTF_TOOLS}/{GENERIC_TEST}/*.py".format(
                MTF_TOOLS=metadata_common.MetadataLoaderMTF.MTF_LINTER_PATH,
-                GENERIC_TEST=common.GENERIC_TEST))
+                GENERIC_TEST=conf.get("generic", "generic_tests")))


Again, either double get or even better getattr.

phracek · 2018-03-06T14:18:31Z

moduleframework/mtf.yaml

+
+# openshift specific section
+openshift:
+  init_wait: 50


Configuration file is a bit long. What about to create a directory /etc/mtf/ and have there several configuration files. One for each type. Ease for modification.

this is good idea, but I'm not sure if anybody than me will modify it :-)
We can discuss it. I like this idea

TomasTomecek · 2018-03-06T14:48:38Z

moduleframework/mtf.yaml

+  # timeout in secs, how long wait for compose (composes are cached, but first round can take long time (createrepo of packages))
+  timeout: 1200
+  new_compose_dict: { sigkeys: [''] }
+  url: "https://odcs.fedoraproject.org"


I expect that internally we'll have a different config.

Yes, it is also mentioned few lines under (location of internal odcs server)
Probably we will ship, or somewhere stored config file for downstream testing

prepare for using private neworks inside nspawn prepare for using private neworks inside nspawn

phracek · 2018-03-07T12:24:39Z

moduleframework/common.py

 from avocado.utils import process
-from moduleframework.mtfexceptions import ModuleFrameworkException, ConfigExc, CmdExc, DefaultConfigExc
+from mtfexceptions import ModuleFrameworkException, ConfigExc, CmdExc, DefaultConfigExc
+from core import DATADIR, is_debug, is_not_silent, print_info, print_debug


Why not only import core. print_info is so general and we don't know from which module comes from.

phracek · 2018-03-07T12:26:25Z

moduleframework/common.py

+
+class MTFConfParser(dict):
+    configdir = "mtf.conf.d"
+    source_cfg = os.path.join(*(["/"] + DATADIR + [configdir]))


Add / to DATADIR and remove ["/"].
I would write
os.path.join(*(DATADIR, [configdir]))

What do you want to do with this code? All directories under DATADIR?

@phracek this is not possible, it is connected with using this in setup.py (there is just relative path, so I cannot add this to this DATADIR list)

phracek · 2018-03-07T12:27:05Z

moduleframework/common.py

+
+    def __init__(self):
+        config = {}
+        for cfgdir in [self.source_cfg, self.default_cfg, self.user_cfg]:


Will this work with several self.source_cfg directories?

phracek · 2018-03-07T12:27:38Z

moduleframework/common.py

+    if is_true(odcstoken):
+        if conf.get("openidc").get("token"):
+            # use value defined in config file if defined
+            return conf["openidc"]["token"]


get and get are missing.

@phracek it is dict, so it is not important to use get, because it is already checked that is defined in if

phracek · 2018-03-07T12:27:48Z

moduleframework/common.py

-            'https://pagure.io/odcs/renew-compose',
-            'https://pagure.io/odcs/delete-compose',
-        ]
+        oidc = openidc_client.OpenIDCClient(*conf["openidc"]["auth"])


the same as above.

phracek · 2018-03-07T12:27:55Z

moduleframework/common.py

-            'https://pagure.io/odcs/delete-compose',
-        ]
+        oidc = openidc_client.OpenIDCClient(*conf["openidc"]["auth"])
+        scopes = conf["openidc"]["scopes"]


@jscotka You did not catch traceback. Is it enough exception for request.exception.HTTPError?

@phracek I think that this is enough. Not important to catch it and transform to another exception. And I've not changed this behaviour anyhow

phracek · 2018-03-07T12:28:03Z

moduleframework/common.py

@@ -348,7 +288,7 @@ def get_profile():
    :return: str
    """

-    return os.environ.get('PROFILE') or MODULE_DEFAULT_PROFILE
+    return os.environ.get('PROFILE') or conf["modularity"]["default_profile"]


@jscotka This returns, either environment variable or value from conf or None. Is it requested?

yes this is expected. or this also can cause traceback in case some user break this value from config. It is expected

phracek · 2018-03-07T12:28:34Z

moduleframework/common.py

@@ -469,7 +409,7 @@ def get_template(self):
        get location of template from config.yaml file
        :return: str
        """
-        return self.info.get(TEMPLATE)
+        return self.info.get(conf["openshift"]["template"])


Default value is missing.

@jscotka Do you count with None if template is not specified?

It has to be specified in this config. There is stored default value and in case user remove this item from config, it causes traceback, it is expected.

phracek · 2018-03-07T12:29:36Z

moduleframework/helpers/openshift_helper.py

@@ -26,7 +26,8 @@
 import random
 import string
 from avocado.utils.process import CmdError
-from moduleframework import common
+from moduleframework.common import conf, is_debug, print_debug, print_info, trans_dict, sanitize_cmd, \


Some parts are part of module core instead of common.

phracek · 2018-03-08T14:37:37Z

moduleframework/common.py

+
+class MTFConfParser(dict):
+    configdir = "mtf.conf.d"
+    source_cfg = os.path.join(*(["/"] + core.DATADIR + [configdir]))


@jscotka What do you want to do with this command?

source_cfg = os.path.join(*(["/"] + ['etc'] + ["yum"])) print source_cfg prints /etc/yum

Why not only os.path.join(core.DATADIR, configdir)?
core.DATADIR could be

core.DATADIR = "/usr/share/moduleframework"

because threre is by intention missing leading / so that it created relative path, instead of /usr/share/moduleframework, and also it is caused by that this syntax was used in setup.py for this directory. So I did not want to change it in setup.py, just put it out of hardcoded value in setup.py
see: https://github.com/fedora-modularity/meta-test-family/pull/218/files#diff-2eeaed663bd0d25b7e608891384b7298L59

phracek · 2018-03-08T14:39:55Z

moduleframework/common.py

-            'https://pagure.io/odcs/renew-compose',
-            'https://pagure.io/odcs/delete-compose',
-        ]
+        oidc = openidc_client.OpenIDCClient(*conf["openidc"]["auth"])


@jscotka Check if ["openidc"]["auth"] exists.
what about if oidc is None. Where do you catch traceback?

I don't want to catch traceback here in case user break config. This behaviour is same for various service, like apache server, in case user write bad config, or break default apache config file, it did not start and probably just lead to some strange error.

phracek · 2018-03-08T14:42:39Z

moduleframework/common.py

@@ -674,7 +612,7 @@ def run(self, command, **kwargs):

    def get_packager(self):
        if not self.packager:
-            self.packager = self.run(PACKAGER_COMMAND, verbose=False).stdout.strip()
+            self.packager = self.run(conf["generic"]["packager_cmd"], verbose=False).stdout.strip()


It will be specified always, right? What if packager_cmd is missing?

it causes traceback. by default it is defined in shipped mtf config. if user remove this. then it will lead to expected traceback.

phracek · 2018-03-08T14:43:09Z

moduleframework/common.py

                cfgfile = "/usr/share/moduleframework/docs/example-config-minimal.yaml"
-                print_debug("Using default minimal config: %s" % cfgfile)
+                core.print_debug("Using default minimal config: %s" % cfgfile)


        try:
            with open(cfgfile, 'r') as ymlfile:
                xcfg = yaml.load(ymlfile.read())
            doc_name = ['modularity-testing', 'meta-test-family', 'meta-test']


Why meta-test?

I don't know. I've defided it looong time ago, as shorter name in config files, it is closed to modularity-testing, it also miss word framework

phracek · 2018-03-08T14:44:45Z

moduleframework/common.py

    return parent


-def get_docker_file(dir_name=DEFAULT_DIR_OF_DOCKER_RELATED_STUFF):
+def get_docker_file(dir_name=conf["docker"]["dockerfiledefaultlocation"]):


This can be None if dockerfiledefaultlocation is empty. Do you count with it?

it will lead to traceback, it is expected

phracek · 2018-03-08T14:45:35Z

moduleframework/compose_info.py

@@ -40,7 +44,7 @@ class ComposeParser(object):

    def __init__(self, compose):
        self.compose = compose
-        xmlrepomd = compose + "/" + REPOMD
+        xmlrepomd = compose + "/" + common.conf["compose"]["repomd"]


Why not os.path.join?

I Know why, it is URL, not directory. It is possible to use os.path.join now, it is equivalent, but in some case os.path.join can lead to another separator like \ at windows, and it causes bad URL

phracek · 2018-03-08T14:46:10Z

moduleframework/core.py

+import sys
+import os
+
+DATADIR = ['usr', 'share', 'moduleframework']


@jscotka Why not /usr/share/moduleframework?

because original usage of this list is in setup.py in this format, copyied from other upstream projects, and I did't want to change this behaviour, just need to have this location in better place that in setup.py.
see https://github.com/fedora-modularity/meta-test-family/pull/218/files#diff-2eeaed663bd0d25b7e608891384b7298L59

phracek · 2018-03-08T14:55:02Z

moduleframework/common.py

+import core
+
+
+class MTFConfParser(dict):


@jscotka What about something like this:

class Config(dict): def __getattr__(self, key): try: return self[key] except KeyError: raise AttributeError def __setattr__(self, key, value): self[key] = value def __init__(self, yaml_file, release="current"): config = yaml.load(yaml_file) self["raw"] = config pdc = config["pdc"] ....

And you have access to all fields.

@phracek does that have some benefits instead of actual implementation to redefine these getters and setters?
I trough to redefine __getattr__ to lead some easier to debug exceptions (like, show path to file where is this found, and show value from default config, what should not be modifed), but it could be further improvement

yeah let's leave it for now.

phracek

LGTM now.

jscotka added the enhancement label Mar 2, 2018

jscotka force-pushed the mtf_config branch 4 times, most recently from 52284c9 to 3863727 Compare March 5, 2018 11:49

TomasTomecek reviewed Mar 5, 2018

View reviewed changes

jscotka force-pushed the mtf_config branch from 3863727 to 6c63680 Compare March 5, 2018 12:34

jscotka changed the title ~~WIP: Mtf config~~ Mtf config file Mar 5, 2018

jscotka requested a review from phracek March 5, 2018 13:20

jscotka force-pushed the mtf_config branch 2 times, most recently from c1d28ce to 24c1c38 Compare March 6, 2018 14:04

phracek requested changes Mar 6, 2018

View reviewed changes

TomasTomecek reviewed Mar 6, 2018

View reviewed changes

jscotka force-pushed the mtf_config branch from b6f98cb to 1ecc79c Compare March 7, 2018 08:14

jscotka added a commit to jscotka/meta-test-family that referenced this pull request Mar 7, 2018

fix issues mentioned in PR fedora-modularity#218 review request

1ecc79c

Use config file for MTF instead of values hardcoded in python code

9f84792

prepare for using private neworks inside nspawn prepare for using private neworks inside nspawn

jscotka force-pushed the mtf_config branch from fb2b5cd to 9f84792 Compare March 7, 2018 10:59

phracek reviewed Mar 7, 2018

View reviewed changes

jscotka force-pushed the mtf_config branch 2 times, most recently from 62c2004 to e2abdb4 Compare March 8, 2018 10:29

fix issues with imports over more levels

2078677

jscotka force-pushed the mtf_config branch from aebcb5a to 2078677 Compare March 8, 2018 11:01

phracek reviewed Mar 8, 2018

View reviewed changes

phracek approved these changes Mar 9, 2018

View reviewed changes

phracek merged commit 30c34e8 into fedora-modularity:master Mar 9, 2018

jscotka mentioned this pull request Mar 9, 2018

DOC: create documentation for configuring MTF tool via config files #223

Open

Mtf config file #218

Mtf config file #218

Conversation

jscotka commented Mar 2, 2018

TomasTomecek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jscotka Mar 5, 2018 • edited Loading

Choose a reason for hiding this comment

phracek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jscotka Mar 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jscotka Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jscotka Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jscotka Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phracek left a comment

Choose a reason for hiding this comment

jscotka Mar 5, 2018 •

edited

Loading

jscotka Mar 6, 2018 •

edited

Loading

jscotka Mar 8, 2018 •

edited

Loading

jscotka Mar 8, 2018 •

edited

Loading

jscotka Mar 8, 2018 •

edited

Loading