Move all defaults from config yaml to defaults.py #218

vasukulkarni · 2019-06-12T18:45:17Z

Signed-off-by: Vasu Kulkarni [email protected]

run_ocsci.py

vasukulkarni · 2019-06-12T19:45:10Z

I will have to run the deployment test to ensure ENV Data is not messed up.

zmc · 2019-06-20T18:25:22Z

ocsci/tests/test_config.py

@@ -16,4 +16,3 @@ def test_defaults(self):
        config_sections = ocsci.config.to_dict().keys()
        for section_name in config_sections:
            section = getattr(ocsci.config, section_name)
-            assert section == getattr(ocs.defaults, section_name)


This makes the test do nothing at all. Why does it need to be changed?

that was old change, just updated it now after separating out defaults.

I'm really confused - your new commits are removing defaults from defaults.py now

some where duplicated in defaults and ENV_DATA, I only removed those duplicates.

vasukulkarni · 2019-06-20T18:44:55Z

Deployment test is passing but have the final oc wait thats causing issue

11:32:14 - MainThread - tests.ecosystem.deployment.test_ocs_basic_install - INFO - Waiting 15 seconds...
11:32:29 - MainThread - utility.utils - INFO - Executing command: oc wait --for condition=ready pod -l app=rook-ceph-operator -n openshift-storage --timeout=120s
11:33:00 - MainThread - utility.utils - DEBUG - CMD output: pod/rook-ceph-operator-5b6856f864-2hcq2 condition met
11:33:00 - MainThread - utility.utils - INFO - Executing command: oc wait --for condition=ready pod -l app=rook-ceph-agent -n openshift-storage --timeout=120s
11:33:00 - MainThread - utility.utils - DEBUG - CMD output: 
11:33:00 - MainThread - utility.utils - ERROR - CMD error:: error: no matching resources found

zmc · 2019-06-20T18:49:23Z

ocs/defaults.py

-    'cli_params': {},
-    'client_version': DEPLOYMENT['installer_version'],
-    'bin_dir': './bin',
-}


This is what I meant - these changes are the opposite of what we agreed on

If you read the comments on the top of the ENV_DATA, it was never supposed to be here in the first place. I am still not clear what specific changes you are talking about, I am removing the data that is not used by any file but exists in defaults.py and removing default values that just exists and used by ENV_DATA

@zmc once this will be merged I will submit other PR or you can finish in your to remove completely the defaut_config.yaml and we can have those vales here and feed the default config.

So not sure now if it's worth to remove them from here if we will return those values here again very soon. But we can just copy paste from old commit so I don't mind. But yep those values are not used anywhere now.

@vasukulkarni specifically, I disagree with removing ENV_DATA, DEPLOYMENT, REPORTING and RUN here.

If the idea is to eventually have all these values in defaults.py, I don't see the point in removing them here. It seems counter-productive. I'd rather see them removed from default_config.yaml, as the goal is to remove that file entirely.

Signed-off-by: Vasu Kulkarni <[email protected]>

vasukulkarni · 2019-06-21T01:15:14Z

Test logs:

18:13:12 - MainThread - tests.conftest - INFO - Waiting 15 seconds...
18:13:27 - MainThread - resources.ocs - INFO - Adding CephFilesystem with name ocsci-cephfs
18:13:27 - MainThread - utility.utils - INFO - Executing command: oc -n openshift-storage --kubeconfig /Users/vasukulkarni/cluster-path/auth/kubeconfig create -f /var/folders/nt/g_hzgqps1q52bc1wbfy3sl_c0000gn/T/CephFilesystem_36kguuv -o yaml
18:13:27 - MainThread - utility.utils - INFO - Executing command: oc -n openshift-storage --kubeconfig /Users/vasukulkarni/cluster-path/auth/kubeconfig get CephFilesystem ocsci-cephfs -o yaml
18:13:28 - MainThread - utility.utils - INFO - Executing command: oc -n openshift-storage --kubeconfig /Users/vasukulkarni/cluster-path/auth/kubeconfig get Pod --selector=app=rook-ceph-mds -o yaml
18:13:31 - MainThread - utility.utils - INFO - Executing command: oc -n openshift-storage --kubeconfig /Users/vasukulkarni/cluster-path/auth/kubeconfig get Pod --selector=app=rook-ceph-mds -o yaml
18:13:35 - MainThread - utility.utils - INFO - Executing command: oc -n openshift-storage --kubeconfig /Users/vasukulkarni/cluster-path/auth/kubeconfig get Pod --selector=app=rook-ceph-mds -o yaml
18:13:38 - MainThread - utility.utils - INFO - Executing command: oc -n openshift-storage --kubeconfig /Users/vasukulkarni/cluster-path/auth/kubeconfig get Pod --selector=app=rook-ceph-mds -o yaml
18:13:42 - MainThread - utility.utils - INFO - Executing command: oc -n openshift-storage --kubeconfig /Users/vasukulkarni/cluster-path/auth/kubeconfig get Pod --selector=app=rook-ceph-mds -o yaml
18:13:45 - MainThread - utility.utils - INFO - Executing command: oc -n openshift-storage --kubeconfig /Users/vasukulkarni/cluster-path/auth/kubeconfig get Pod --selector=app=rook-ceph-mds -o yaml
18:13:49 - MainThread - utility.utils - INFO - Executing command: oc -n openshift-storage --kubeconfig /Users/vasukulkarni/cluster-path/auth/kubeconfig get Pod --selector=app=rook-ceph-mds -o yaml
18:13:53 - MainThread - utility.utils - INFO - Executing command: oc -n openshift-storage --kubeconfig /Users/vasukulkarni/cluster-path/auth/kubeconfig get Pod --selector=app=rook-ceph-mds -o yaml
18:13:53 - MainThread - utility.utils - INFO - Executing command: oc -n openshift-storage --kubeconfig /Users/vasukulkarni/cluster-path/auth/kubeconfig get CephFileSystem  -o yaml
18:13:54 - MainThread - utility.utils - INFO - Executing command: oc -n openshift-storage --kubeconfig /Users/vasukulkarni/cluster-path/auth/kubeconfig get Pod --selector=app=rook-ceph-tools -o yaml
18:13:54 - MainThread - tests.helpers - INFO - ocsci-cephfs
18:13:54 - MainThread - utility.utils - INFO - Executing command: oc -n openshift-storage --kubeconfig /Users/vasukulkarni/cluster-path/auth/kubeconfig rsh rook-ceph-tools-76c7d559b6-7qfl2 ceph fs ls --format json-pretty
18:13:56 - MainThread - tests.helpers - INFO - ocsci-cephfs
18:13:56 - MainThread - tests.helpers - INFO - FileSystem got created from Ceph Side
18:13:56 - MainThread - utility.utils - INFO - Executing command: oc -n openshift-storage --kubeconfig /Users/vasukulkarni/cluster-path/auth/kubeconfig get CephFileSystem ocsci-cephfs -o yaml
18:13:57 - MainThread - tests.helpers - INFO - Filesystem got created from kubernetes Side
18:13:57 - MainThread - tests.conftest - INFO - MDS deployment is successful!
18:13:57 - MainThread - tests.conftest - INFO - Done creating rook resources, waiting for HEALTH_OK
18:13:57 - MainThread - utility.utils - INFO - Executing command: oc wait --for condition=ready pod -l app=rook-ceph-tools -n openshift-storage --timeout=120s
18:13:57 - MainThread - utility.utils - INFO - Executing command: oc -n openshift-storage get pod -l 'app=rook-ceph-tools' -o jsonpath='{.items[0].metadata.name}'
18:13:58 - MainThread - utility.utils - INFO - Executing command: oc -n openshift-storage exec rook-ceph-tools-76c7d559b6-7qfl2 ceph health
18:14:00 - MainThread - tests.conftest - INFO - HEALTH_OK, install successful.
--------------------------------------------------------------------------------------------- live log call ----------------------------------------------------------------------------------------------
18:14:00 - MainThread - oc.openshift_ops - INFO - Testing access to cluster with /Users/vasukulkarni/cluster-path/auth/kubeconfig
18:14:00 - MainThread - utility.utils - INFO - Executing command: oc cluster-info
18:14:00 - MainThread - oc.openshift_ops - INFO - Access to cluster is OK!
PASSED                                                                                                                                                                                             [100%]

=============================================================================== 1 passed, 18 deselected in 1983.57 seconds ===============================================================================

conf/ocsci/default_config.yaml

conf/README.md

conf/ocsci/default_config.yaml

petr-balogh · 2019-06-20T10:38:32Z

ocs/defaults.py

-module, plese put your defaults rather to mentioned default_config.yaml file!
-See the documentation in conf/README.md file to understand this config file.
+Defaults module. All the defaults used by OSCCI framework should
+reside in this module.


We should mention that all defaults values are suppose to be unchanged during whole execution as we consider them as defaults. We are also having some discussion on #259 about defaults.py contains a lot of variables which probably should live in different place, this we can discuss on today's call.

yeah this we can take as a different topic.

@petr-balogh could you explain why you think the values in defaults.py shouldn't be changed? It's my impression that a defaults.py sort of module should contain every configurable option and is expected to be overridden by the end-user if they need it to be.

conf/ocsci/default_config.yaml

Signed-off-by: Vasu Kulkarni <[email protected]>

zmc · 2019-06-21T15:24:54Z

ocs/defaults.py

-    'cli_params': {},
-    'client_version': DEPLOYMENT['installer_version'],
-    'bin_dir': './bin',
-}


@vasukulkarni specifically, I disagree with removing ENV_DATA, DEPLOYMENT, REPORTING and RUN here.

If the idea is to eventually have all these values in defaults.py, I don't see the point in removing them here. It seems counter-productive. I'd rather see them removed from default_config.yaml, as the goal is to remove that file entirely.

zmc · 2019-06-21T15:25:36Z

ocsci/tests/test_config.py

+        run_data = getattr(ocsci.config, 'RUN')
+        assert env_data['rook_image'] == 'rook/ceph:master'
+        assert reporting_data['email']['address'] == '[email protected]'
+        assert run_data['bin_dir'] == './bin'


I think the existing test should be left alone, and a new test added if you want to verify specific values.

The old assert where it tries to do "assert section == getattr(ocs.defaults, section_name)" is not valid since it would expect you to have values in default_config.yaml as well as define in defaults.py, The purpose of the PR was to remove the duplicates.

vasukulkarni · 2019-06-21T15:46:20Z

@zmc the dropping of default_config.yaml came up recently, I am not clear on the ideas, so it has to be a follow up PR.

zmc

Attempting to summarize a long BlueJeans call briefly:

The term 'defaults' wasn't meaning quite the same thing to all of us
Part of what is in defaults.py is actual configuration defaults, while part of it is actually CRD files that test cases deepcopy and extend for their use cases
This PR wants to make default_config.yaml the canonical location of the configuration defaults, leaving the CRDs (and a little leftover cruft) in defaults.py
defaults.py should be renamed to something that better reflects its content, or removed entirely in favor of Avoid the pattern of doing a deepcopy of global data in tests #276 - but not in this PR

Based on the above, 👍

vasukulkarni requested a review from a team as a code owner June 12, 2019 18:45

petr-balogh reviewed Jun 12, 2019

View reviewed changes

run_ocsci.py Outdated Show resolved Hide resolved

vasukulkarni force-pushed the wip-fix-defaults branch from bc0475a to 114b0e7 Compare June 12, 2019 19:44

vasukulkarni added the Needs Testing Run tests and provide logs link label Jun 12, 2019

This was referenced Jun 17, 2019

Force openshift binaries download and log versions #180

Merged

adds a pytest session fixture to automatically deploy a test cluster #173

Merged

vasukulkarni force-pushed the wip-fix-defaults branch from 114b0e7 to b49a7cf Compare June 19, 2019 20:42

vasukulkarni mentioned this pull request Jun 19, 2019

remove tests_config which was added with wrong assumption #251

Closed

zmc mentioned this pull request Jun 19, 2019

Move config initialization to Config class #259

Merged

zmc requested changes Jun 20, 2019

View reviewed changes

vasukulkarni force-pushed the wip-fix-defaults branch from 7b58e93 to 956f7eb Compare June 20, 2019 18:31

vasukulkarni added Verified Mark when PR was verified and log provided and removed Needs Testing Run tests and provide logs link labels Jun 20, 2019

zmc requested changes Jun 20, 2019

View reviewed changes

vasukulkarni force-pushed the wip-fix-defaults branch from e81bd1d to 5ea445b Compare June 21, 2019 00:17

Move all defaults from config yaml to defaults.py

de659e0

Signed-off-by: Vasu Kulkarni <[email protected]>

vasukulkarni force-pushed the wip-fix-defaults branch from 5ea445b to de659e0 Compare June 21, 2019 00:22

petr-balogh reviewed Jun 21, 2019

View reviewed changes

conf/ocsci/default_config.yaml Show resolved Hide resolved

conf/ocsci/default_config.yaml Show resolved Hide resolved

Fix review comments

5813f53

Signed-off-by: Vasu Kulkarni <[email protected]>

petr-balogh approved these changes Jun 21, 2019

View reviewed changes

zmc requested changes Jun 21, 2019

View reviewed changes

zmc approved these changes Jun 21, 2019

View reviewed changes

vasukulkarni merged commit 89a4398 into master Jun 21, 2019

zmc mentioned this pull request Jun 26, 2019

Move default_config.yaml to python defaults module #295

Closed

vasukulkarni deleted the wip-fix-defaults branch June 29, 2019 22:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move all defaults from config yaml to defaults.py #218

Move all defaults from config yaml to defaults.py #218

vasukulkarni commented Jun 12, 2019

vasukulkarni commented Jun 12, 2019

zmc Jun 20, 2019

vasukulkarni Jun 20, 2019

zmc Jun 20, 2019

vasukulkarni Jun 20, 2019

vasukulkarni commented Jun 20, 2019

zmc Jun 20, 2019

vasukulkarni Jun 20, 2019

petr-balogh Jun 21, 2019

zmc Jun 21, 2019

vasukulkarni commented Jun 21, 2019

petr-balogh Jun 20, 2019

vasukulkarni Jun 21, 2019

petr-balogh Jun 21, 2019

andrewschoen Jun 21, 2019

zmc Jun 21, 2019

zmc Jun 21, 2019

vasukulkarni Jun 21, 2019

vasukulkarni commented Jun 21, 2019

zmc left a comment

Move all defaults from config yaml to defaults.py #218

Move all defaults from config yaml to defaults.py #218

Conversation

vasukulkarni commented Jun 12, 2019

vasukulkarni commented Jun 12, 2019

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

vasukulkarni commented Jun 20, 2019

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

vasukulkarni commented Jun 21, 2019

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

vasukulkarni commented Jun 21, 2019

zmc left a comment

Choose a reason for hiding this comment