Skip to content
This repository has been archived by the owner on Apr 7, 2022. It is now read-only.

[WIP] Dynaconf for yamls #9376

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

[WIP] Dynaconf for yamls #9376

wants to merge 8 commits into from

Conversation

kedark3
Copy link
Contributor

@kedark3 kedark3 commented Sep 18, 2019 •
edited
Loading

Purpose or Intent

This PR is an enhancement of what I have done in PR #9315 as it is able to read values for all the available yamls in conf/ directory.

It does have limitations though(dynaconf imposed):

  1. It cannot override and merge values from .local.yaml files
    [Update] I have a PR that may fix the issue number 1 listed above: https://github.com/rochacbruno/dynaconf/pull/238 - waiting on them to release new version
  2. It does not have a property runtime as the conf had
    [Update] I have made it possible to mimic the runtime property with dynaconf and it seems to collect alright locally.

You still need my latest GitLab PR to make this code work locally.

File names _copy are not final, will rename files before merge.

@dajoRH dajoRH added the lint-ok label Sep 18, 2019
john-dupuy
john-dupuy reviewed Sep 19, 2019
View reviewed changes
Copy link
Contributor

@john-dupuy john-dupuy left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kedark3 can you provide some comments on how to test this PR locally? Is it as simple as checking out this PR, checkout our your yamls PR and then doing

from cfme.utils import conf_copy
print(conf_copy.cfme_data)

I get an KeyError when I try to do conf_copy.cfme_data.basic_info?

@kedark3
Copy link
Contributor Author

kedark3 commented Sep 19, 2019

@john-dupuy yes please pull my YAMLs PR 832 and then you can run:

In [1]: from cfme.utils import conf_copy                                                                               
In [2]: conf_copy.cfme_data                                                                                            
Out[2]: <cfme.utils.config_copy.ConfigData at 0x7f9aa6e7c1d0>

In [3]: conf_copy.cfme_data.basic_info <or any other keys>

@kedark3 kedark3 mentioned this pull request Sep 20, 2019
Closed
sbulage
sbulage suggested changes Sep 23, 2019
View reviewed changes
Copy link
Contributor

@sbulage sbulage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One Suggestion, Else LGTM 👍

cfme/utils/config_copy.py Outdated

class Configuration(object):
"""
holds the current configuration
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion:
Can you please elaborate more? with example if possible.?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some comments.

@kedark3
Copy link
Contributor Author

kedark3 commented Sep 23, 2019

I am waiting on my Dynaconf PR before working on this one. In the meanwhile feel free to leave reviews. I will fix travis later.

@john-dupuy
Copy link
Contributor

Moving to WIPTEST based off of #9376 (comment), let me know if you'd like to leave it RFR @kedark3

@john-dupuy john-dupuy changed the title [RFR]Dynaconf for yamls [WIPTEST] Dynaconf for yamls Sep 24, 2019
@kedark3 kedark3 force-pushed the dynaconf_for_yamls branch 4 times, most recently from f5c230c to b53649f Compare October 2, 2019 19:51
@kedark3 kedark3 changed the title [WIPTEST] Dynaconf for yamls [RFR] Dynaconf for yamls Oct 9, 2019
@kedark3
Copy link
Contributor Author

kedark3 commented Oct 9, 2019

To Reviewers: This PR passed the jenkins run, I can point you to it if needed.Kindly review.

@dajoRH dajoRH removed the WIP-testing label Oct 9, 2019
john-dupuy
john-dupuy suggested changes Oct 11, 2019
View reviewed changes
Copy link
Contributor

@john-dupuy john-dupuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few small questions, but looks great! Nice work KK! 🎉

Travis build is timing out, waiting for travis to be fixed before moving to 1LP.

cfme/utils/log.py Outdated
except KeyError:
# env not loaded yet
pass
# try:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kedark3 do you want to remove these lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removing it now as I see no effect of this in test run on jenkins.

cfme/utils/config.py Outdated
"""
if self.settings is None:
self.settings = {
file.basename[:-5]: ConfigData(env=file.basename[:-5])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the -5? Maybe add a comment here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.yml is also a valid file type, this is a bad practice.

Since file here is a Py.path.local instance, you want to use file.purebasename.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

s = {f.purebasename: 'a value' for f in conf_path.listdir() if '.local.yaml' not in f.basename and f.ext in ['.yaml', '.yml']}

Purebasename alone gets tripped up by the .local.yaml files, (as you pointed out in chat! :) ) but you should be fine using it like above since you're filtering the files anyway within the comprehension.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed this

cfme/fixtures/parallelizer/remote.py Outdated Show resolved Hide resolved
@kedark3 kedark3 force-pushed the dynaconf_for_yamls branch from 3db5b3c to f3768c6 Compare October 11, 2019 15:13
mshriver
mshriver reviewed Oct 11, 2019
View reviewed changes
cfme/utils/config.py Outdated
'perf_tests': <cfme.utils.config_copy.ConfigData at 0x7f2a943ad2e8>}

"""
if self.settings is None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use a cached_property? That appears to be what this method is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

mshriver
mshriver reviewed Oct 11, 2019
View reviewed changes
cfme/utils/config.py Outdated

:param name: name of the configuration object
"""
self.configure()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to explicitly call this when the data that its filling is provided by the cached_property on access.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

@kedark3 kedark3 force-pushed the dynaconf_for_yamls branch from f3768c6 to df73523 Compare October 11, 2019 16:11
@dajoRH dajoRH changed the title [RFR] Dynaconf for yamls [WIP] Dynaconf for yamls Oct 11, 2019
@kedark3 kedark3 force-pushed the dynaconf_for_yamls branch from df73523 to 49751d6 Compare October 11, 2019 17:07
@kedark3 kedark3 force-pushed the dynaconf_for_yamls branch from 49751d6 to d3623c5 Compare October 11, 2019 17:15
@kedark3 kedark3 force-pushed the dynaconf_for_yamls branch from d3623c5 to b60bf6c Compare October 11, 2019 17:32
kedark3
kedark3 commented Oct 11, 2019
View reviewed changes
requirements/frozen.txt
@@ -279,6 +287,7 @@ PyYAML==5.1
pyzmq==18.0.1
qtconsole==4.4.3
redfish-client==0.1.0
redis==3.3.10
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added as part of dynaconf requirements . https://github.com/rochacbruno/dynaconf/blob/master/setup.py

@kedark3 kedark3 force-pushed the dynaconf_for_yamls branch from b60bf6c to 3f167ce Compare October 11, 2019 18:52
@dajoRH
Copy link
Contributor

dajoRH commented Oct 16, 2019

Would you mind rebasing this Pull Request against latest master, please? :trollface:
CFME QE Bot

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mshriver mshriver mshriver left review comments

@john-dupuy john-dupuy john-dupuy requested changes

@sbulage sbulage sbulage requested changes

Assignees
No one assigned
Labels
enhancement lint-ok needs-rebase tech-debt WIP
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kedark3 @john-dupuy @dajoRH @sbulage @mshriver