Refactor Nulecule config. #720
Conversation
| """ | ||
| context = {} | ||
|
|
||
| context.update(copy.copy(self._data[GLOBAL_CONF])) |
There was a problem hiding this comment.
can we use copy.deepcopy, just to be on safe side?
There was a problem hiding this comment.
It's only one level, so I did not do copy.deepcopy
|
|
||
| nc = NuleculeComponent('some-app', 'some/path', params=params) | ||
| nc.load_config(config=copy.deepcopy(initial_config)) | ||
| nc.load_config(config=config) | ||
| from ipdb import set_trace; set_trace() |
|
@rtnpro there are some issues while using this PR (atomic) ✔ ~/git/nulecule-library/etherpad-centos7-atomicapp [master ↑·39|✔]
00:06 $ sudo atomicapp run . -v
1463457985 - [INFO] - cli/main.py - Action/Mode Selected is: run
1463457985 - [DEBUG] - cli/main.py - Final parsed cmdline: run . -v
1463457985 - [DEBUG] - nulecule/main.py - NuleculeManager init app_path: .
1463457985 - [DEBUG] - nulecule/main.py - NuleculeManager init image: None
1463457985 - [DEBUG] - nulecule/main.py - Request to unpack to None to .
1463457985 - [DEBUG] - atomicapp/plugin.py - Loading providers from /home/vagrant/atomicapp/atomicapp/providers
1463457985 - [DEBUG] - atomicapp/plugin.py - Loading providers from /home/vagrant/atomicapp/atomicapp/providers
1463457985 - [INFO] - nulecule/base.py - Pulling external application for mariadb-centos7-atomicapp.
1463457985 - [INFO] - nulecule/base.py - Unpacking image: projectatomic/mariadb-centos7-atomicapp to ./external/mariadb-centos7-atomicapp
[SNIP]
1463457989 - [DEBUG] - nulecule/main.py - FILE: ./answers.conf.gen
1463457989 - [DEBUG] - nulecule/main.py - ANSWERS: defaultdict(<type 'dict'>, {u'mariadb-centos7-atomicapp.mariadb-atomicapp': {u'db_pass': 'q', u'db_name': 'q', u'root_pass': u'MySQLPass', u'db_user': 'q'}, u'mariadb-centos7-atomicapp': {u'provider': u'kubernetes'}, u'etherpad-app': {u'db_pass': 'q', u'db_name': 'q', u'db_host': u'mariadb', u'db_user': 'q', u'image': u'centos/etherpad', u'hostport': 9001}, 'general': {'namespace': 'default', u'provider': 'kubernetes'}})
Your application resides in .
Please use this directory for managing your application
(atomic) ✔ ~/git/nulecule-library/etherpad-centos7-atomicapp [master ↑·39|…14]
00:06 $ sudo atomicapp stop . -v
1463458001 - [INFO] - cli/main.py - Action/Mode Selected is: stop
1463458001 - [DEBUG] - cli/main.py - Final parsed cmdline: stop . -v
1463458001 - [DEBUG] - nulecule/main.py - NuleculeManager init app_path: .
1463458001 - [DEBUG] - nulecule/main.py - NuleculeManager init image: None
1463458001 - [DEBUG] - atomicapp/utils.py - Loading answers from file: ./answers.conf.gen
1463458001 - [ERROR] - cli/main.py - AnyMarkupError: caught <class 'yaml.parser.ParserError'>: expected '<document start>', but found '<scalar>'
in "<file>", line 2, column 1
Original traceback:
Traceback (most recent call last):
File "build/bdist.linux-x86_64/egg/anymarkup_core/__init__.py", line 110, in parse
res = _do_parse(proper_inp, fmt, encoding, force_types)
File "build/bdist.linux-x86_64/egg/anymarkup_core/__init__.py", line 252, in _do_parse
res = yaml.safe_load(inp)
File "build/bdist.linux-x86_64/egg/yaml/__init__.py", line 93, in safe_load
return load(stream, SafeLoader)
File "build/bdist.linux-x86_64/egg/yaml/__init__.py", line 71, in load
return loader.get_single_data()
File "build/bdist.linux-x86_64/egg/yaml/constructor.py", line 37, in get_single_data
node = self.get_single_node()
File "build/bdist.linux-x86_64/egg/yaml/composer.py", line 39, in get_single_node
if not self.check_event(StreamEndEvent):
File "build/bdist.linux-x86_64/egg/yaml/parser.py", line 98, in check_event
self.current_event = self.state()
File "build/bdist.linux-x86_64/egg/yaml/parser.py", line 174, in parse_document_start
self.peek_token().start_mark)
ParserError: expected '<document start>', but found '<scalar>'
in "<file>", line 2, column 1
Traceback (most recent call last):
File "/home/vagrant/atomicapp/atomicapp/cli/main.py", line 130, in cli_func_exec
cli_func(cli_func_args)
File "/home/vagrant/atomicapp/atomicapp/cli/main.py", line 97, in cli_stop
nm.stop(**argdict)
File "/home/vagrant/atomicapp/atomicapp/nulecule/main.py", line 341, in stop
self._process_answers()
File "/home/vagrant/atomicapp/atomicapp/nulecule/main.py", line 396, in _process_answers
self.answers = Utils.loadAnswers(self.answers_file)
File "/home/vagrant/atomicapp/atomicapp/utils.py", line 367, in loadAnswers
return anymarkup.parse_file(answers_file)
File "build/bdist.linux-x86_64/egg/anymarkup_core/__init__.py", line 139, in parse_file
return parse(f, format, encoding, force_types)
File "build/bdist.linux-x86_64/egg/anymarkup_core/__init__.py", line 113, in parse
raise AnyMarkupError(e, traceback.format_exc())
AnyMarkupError: AnyMarkupError: caught <class 'yaml.parser.ParserError'>: expected '<document start>', but found '<scalar>'
in "<file>", line 2, column 1
Original traceback:
Traceback (most recent call last):
File "build/bdist.linux-x86_64/egg/anymarkup_core/__init__.py", line 110, in parse
res = _do_parse(proper_inp, fmt, encoding, force_types)
File "build/bdist.linux-x86_64/egg/anymarkup_core/__init__.py", line 252, in _do_parse
res = yaml.safe_load(inp)
File "build/bdist.linux-x86_64/egg/yaml/__init__.py", line 93, in safe_load
return load(stream, SafeLoader)
File "build/bdist.linux-x86_64/egg/yaml/__init__.py", line 71, in load
return loader.get_single_data()
File "build/bdist.linux-x86_64/egg/yaml/constructor.py", line 37, in get_single_data
node = self.get_single_node()
File "build/bdist.linux-x86_64/egg/yaml/composer.py", line 39, in get_single_node
if not self.check_event(StreamEndEvent):
File "build/bdist.linux-x86_64/egg/yaml/parser.py", line 98, in check_event
self.current_event = self.state()
File "build/bdist.linux-x86_64/egg/yaml/parser.py", line 174, in parse_document_start
self.peek_token().start_mark)
ParserError: expected '<document start>', but found '<scalar>'
in "<file>", line 2, column 1another one (atomic) ✘-1 ~/git/nulecule-library/apache-centos7-atomicapp [master ↑·39|…17]
00:03 $ sudo atomicapp run . -v
1463457814 - [INFO] - cli/main.py - Action/Mode Selected is: run
1463457814 - [DEBUG] - cli/main.py - Final parsed cmdline: run . -v
[SNIP]
1463457814 - [INFO] - providers/kubernetes.py - found kubectl at /usr/bin/kubectl
1463457814 - [INFO] - providers/kubernetes.py - Deploying to Kubernetes
1463457814 - [DEBUG] - providers/kubernetes.py - ./artifacts/kubernetes/.apache-centos7-atomicapp-pod.json
1463457814 - [DEBUG] - atomicapp/utils.py -
<<< stdout >>>
pod "apache-centos7-atomicapp" created
<<< end >>>
1463457814 - [DEBUG] - atomicapp/utils.py -
<<< stderr >>>
<<< end >>>
1463457814 - [DEBUG] - nulecule/main.py - Writing answers to file.
1463457814 - [DEBUG] - nulecule/main.py - FILE: ./answers.conf.gen
1463457814 - [DEBUG] - nulecule/main.py - ANSWERS: defaultdict(<type 'dict'>, {u'apache-centos7-atomicapp': {u'image': u'centos/httpd', u'hostport': 80}, 'general': {'namespace': 'default', u'provider': 'kubernetes'}})
Your application resides in .
Please use this directory for managing your application
(atomic) ✔ ~/git/nulecule-library/apache-centos7-atomicapp [master ↑·39|…18]
00:03 $ sudo atomicapp stop .
[INFO] - main.py - Action/Mode Selected is: stop
[ERROR] - main.py - 'dict' object has no attribute 'set'
Traceback (most recent call last):
File "/home/vagrant/atomicapp/atomicapp/cli/main.py", line 130, in cli_func_exec
cli_func(cli_func_args)
File "/home/vagrant/atomicapp/atomicapp/cli/main.py", line 97, in cli_stop
nm.stop(**argdict)
File "/home/vagrant/atomicapp/atomicapp/nulecule/main.py", line 346, in stop
self.nulecule.load_config(config=self.answers)
File "/home/vagrant/atomicapp/atomicapp/nulecule/base.py", line 242, in load_config
config=config, ask=ask, skip_asking=skip_asking)
File "/home/vagrant/atomicapp/atomicapp/nulecule/lib.py", line 73, in load_config
config.set(param[NAME_KEY], value)
AttributeError: 'dict' object has no attribute 'set' |
|
@rtnpro one more thing, I am not able to retrieve the command line parameters from the config object (atomic) ✔ ~/git/nulecule-library/etherpad-centos7-atomicapp [master ↑·39|…11]
01:21 $ sudo atomicapp run . -v --provider docker
1463462467 - [INFO] - cli/main.py - Action/Mode Selected is: run
[SNIP]
1463462467 - [DEBUG] - atomicapp/plugin.py - Loading providers from /home/vagrant/atomicapp/atomicapp/providers
==> db_user (Database User): q
==> db_pass (Database Password): a
==> db_name (Database Name): z
==> db_user (Database User): x
==> db_pass (Database Password): s
==> db_name (Database Name): w
[SNIP]
1463462476 - [DEBUG] - providers/docker.py - Given config: {u'db_pass': 'a', u'db_name': 'z', u'root_pass': u'MySQLPass', u'db_user': 'q', u'provider': u'kubernetes', 'namespace': 'default'}
1463462476 - [DEBUG] - providers/docker.py - Namespace: default
1463462476 - [WARNING] - providers/docker.py - The artifact name has not been provided within Nulecule, using a UUID instead
1463462476 - [DEBUG] - providers/docker.py - No image name found for artifact, using UUID aa7ea1b10b75 in container name
> /home/vagrant/atomicapp/atomicapp/nulecule/base.py(345)run()
344 import ipdb; ipdb.set_trace()
--> 345 provider.run()
346
ipdb> import pprint
ipdb> pprint.pprint(self.config.__dict__)
{'_answers': defaultdict(<type 'dict'>, {u'mariadb-centos7-atomicapp.mariadb-atomicapp': {}, u'mariadb-centos7-atomicapp': {}, 'general': {'namespace': 'default'}}),
'_cli': {},
'_context': {u'db_name': 'z',
u'db_pass': 'a',
u'db_user': 'q',
'namespace': 'default',
u'provider': u'kubernetes',
u'root_pass': u'MySQLPass'},
'_current_ns': u'mariadb-atomicapp',
'_data': defaultdict(<type 'dict'>, {u'mariadb-centos7-atomicapp.mariadb-atomicapp': {u'db_pass': 'a', u'db_name': 'z', u'db_user': 'q', u'root_pass': u'MySQLPass'}, u'mariadb-centos7-atomicapp': {u'provider': u'kubernetes'}, u'etherpad-app': {u'image': u'centos/etherpad', u'db_pass': 's', u'db_name': 'w', u'db_user': 'x', u'db_host': u'mariadb', u'hostport': 9001}, 'general': {u'provider': u'kubernetes'}}),
'_is_nulecule': False,
'_namespace': u'mariadb-centos7-atomicapp.mariadb-atomicapp',
'_parent_ns': u'mariadb-centos7-atomicapp',
'_provider': None}
ipdb> c
1463462527 - [INFO] - providers/docker.py - Deploying to provider: Docker
1463462527 - [INFO] - providers/docker.py - WARNING: Using --name provided within artifact file.
04d6972f9585820750e3e18a22593f66a769c6a5fc7da4640bff21737568390b
1463462527 - [DEBUG] - atomicapp/plugin.py - Found provider <class 'docker.DockerProvider'>
1463462527 - [WARNING] - atomicapp/plugin.py - Configuration option 'provider-config' not found
1463462527 - [DEBUG] - providers/docker.py - Given config: {u'db_pass': 's', u'db_name': 'w', u'db_host': u'mariadb', u'db_user': 'x', u'provider': u'kubernetes', u'image': u'centos/etherpad', u'hostport': 9001, 'namespace': 'default'}
1463462527 - [DEBUG] - providers/docker.py - Namespace: default
> /home/vagrant/atomicapp/atomicapp/nulecule/base.py(345)run()
344 import ipdb; ipdb.set_trace()
--> 345 provider.run()
346
ipdb> import pprint
ipdb> pprint.pprint(self.config.__dict__)
{'_answers': defaultdict(<type 'dict'>, {u'etherpad-app': {}, 'general': {'namespace': 'default'}}),
'_cli': {},
'_context': {u'db_host': u'mariadb',
u'db_name': 'w',
u'db_pass': 's',
u'db_user': 'x',
u'hostport': 9001,
u'image': u'centos/etherpad',
'namespace': 'default',
u'provider': u'kubernetes'},
'_current_ns': u'etherpad-app',
'_data': defaultdict(<type 'dict'>, {u'mariadb-centos7-atomicapp.mariadb-atomicapp': {u'db_pass': 'a', u'db_name': 'z', u'db_user': 'q', u'root_pass': u'MySQLPass'}, u'mariadb-centos7-atomicapp': {u'provider': u'kubernetes'}, u'etherpad-app': {u'image': u'centos/etherpad', u'db_pass': 's', u'db_name': 'w', u'db_user': 'x', u'db_host': u'mariadb', u'hostport': 9001}, 'general': {u'provider': u'kubernetes'}}),
'_is_nulecule': False,
'_namespace': u'etherpad-app',
'_parent_ns': '',
'_provider': None}
ipdb> c
1463462556 - [INFO] - providers/docker.py - Deploying to provider: Docker
1463462556 - [INFO] - providers/docker.py - WARNING: Using --name provided within artifact file.
181242ae722bf65e2d0c94b38a717058a3d56dc3c4bc711a0fcade238ca1dc51
1463462557 - [DEBUG] - nulecule/main.py - Writing answers to file.
1463462557 - [DEBUG] - nulecule/main.py - FILE: ./answers.conf.gen
1463462557 - [DEBUG] - nulecule/main.py - ANSWERS: defaultdict(<type 'dict'>, {u'mariadb-centos7-atomicapp.mariadb-atomicapp': {u'db_pass': 'a', u'db_name': 'z', u'root_pass': u'MySQLPass', u'db_user': 'q'}, u'mariadb-centos7-atomicapp': {u'provider': u'kubernetes'}, u'etherpad-app': {u'db_pass': 's', u'db_name': 'w', u'db_host': u'mariadb', u'db_user': 'x', u'image': u'centos/etherpad', u'hostport': 9001}, 'general': {'namespace': 'default', u'provider': u'kubernetes'}})
Your application resides in .
Please use this directory for managing your application |
| Returns: | ||
| Current namespace (str). | ||
| """ | ||
| return self._namespace or GLOBAL_CONF |
There was a problem hiding this comment.
We have a constant named DEFAULT_NAMESPACE why not use that, instead of GLOBAL_CONF?
There was a problem hiding this comment.
@surajssd DEFAULT_NAMESPACE and GLOBAL_CONF are different. GLOBAL_CONF stands for the general section in answers file.
rtnpro
force-pushed
the
refactor-config
branch
3 times, most recently
from
40f0f4b to
c9474e7
Compare
|
| self.nulecule.load_config(config=self.nulecule.config, ask=ask) | ||
| self.nulecule.load_config(ask=ask) | ||
| if cli_provider: | ||
| self.nulecule.config.set('provider', cli_provider) | ||
| self.nulecule.render(cli_provider, dryrun) | ||
| self.nulecule.run(cli_provider, dryrun) |
There was a problem hiding this comment.
changing above to something like this can help -> self.nulecule.run(self.nulecule.config.set('provider', cli_provider), dryrun)
If we do it here it removes the requirement of making checks at all here: https://github.com/projectatomic/atomicapp/pull/720/files#diff-093821a6e6f49d13d49fdcb4f911695dR93
|
On Wed, May 25, 2016 at 2:19 PM, Suraj Deshmukh
We can take that into account. But, we shouldn't make the CLI too complex. Ratnadeep Debnath, |
We don't need to make CLI or any thing any complex, all that can be done is put one more return somehow in |
|
Hey @rtnpro - Should we make the new Config class as generic as possible and then add features to it to be able to do what we want? Right now we have arguments to
So we could have something like: we could also have a config.getvalallsources(key='provider') that would return something like: This function would be used when we needed to do some complex logic based on what value was provided from what source. The point is that the config class should be dumb and not know too much about the data it is handling, but just handle the data well. A natural question is: where do we store that logic then? We can easily have a Config Class and then an AtomicAppConfig class that takes advantage of it and does higher level things like setting up the sources, etc.. Does that make sense? Am I crazy? Don't answer that last one :) |
| @@ -78,6 +78,7 @@ def __init__(self, id, specversion, graph, basepath, metadata=None, | |||
| params (list): List of params for the Nulecule application | |||
| config (dict): Config data for the Nulecule application | |||
| namespace (str): Namespace of the current Nulecule application | |||
| cli (dict): CLI data | |||
There was a problem hiding this comment.
I think this is the wrong way to go about it. We are already processing what we call cli_answers in the cli
We do need to process data from the CLI, but I don't know if this is the right place. We should unify around this approach and what we are already doing.
There was a problem hiding this comment.
Ah woops, my recent comment is the same as what @dustymabe just said about this PR. I agree with him too. Should only be passing it in once.
|
On Thu, May 26, 2016 at 2:15 AM, Dusty Mabe [email protected] wrote:
I buy into this idea 👍
What about the |
TBH I think the "namespace" term is completely overloaded. We have "namespace" in the providers (kubernetes/openshift), we have "namespace" in the INI config files, etc.. Can we choose a new term that would help to disambiguate? It would also help if we had a short document that explained the strategy with the handling of config data. i.e. whatever new term we come up with for how we are going to handle "namespaces" in the config, let's write a short design document for how we are going to handle this stuff so people can read it and get the big picture before they dive into the code. |
|
@cdrage @kadel could you review this PR? I am reluctant to put too much effort to make the As per @dustymabe's proposal, it will not be enough to just have sources, we need to have another layer for @cdrage @kadel if you are OK with the code, I can fix the tests and get it merge ready. |
| @@ -51,7 +51,7 @@ def print_app_location(app_path): | |||
| def cli_genanswers(args): | |||
| argdict = args.__dict__ | |||
| nm = NuleculeManager(app_spec=argdict['app_spec'], | |||
| destination='none') | |||
| destination='none', cli=argdict) | |||
There was a problem hiding this comment.
Errrr...
argdict is already passed in nm.genanswers(**argdict)
Why do we need to pass it again? Could you change it so we simply pass it via NuleculeManager and remove the need to pass it within nm.genanswers(**argdict)?
rtnpro
force-pushed
the
refactor-config
branch
from
058b814 to
35c043d
Compare
|
#dotestdocker |
|
#dotests |
| result = anymarkup.parse_file(answers_file, format=format) | ||
| except anymarkup.AnyMarkupError: | ||
| result = anymarkup.parse_file(answers_file) | ||
| return result |
There was a problem hiding this comment.
what are we fixing here?
There was a problem hiding this comment.
so, when we have : or . in the section names in an INI file, anymarkup does not identify it as an INI file by default, so, we have to explicitly mention the fromat when parsing the file. This L370 also works well if the user specifies a non default answers format, as well.
L372 is to handle the scenario when the user points to a JSON/YAML answers file but does not set the answers format, which defaults to INI. In such a case, L370 won't be able to load the file inspite of the fact that the file is a proper JSON, YAML file.
There was a problem hiding this comment.
a couple of things:
- this would be good to add in as a comment because no one else would figure that out
- the fact that anymarkup does not detect an INI file if it has
:- is that a bug in anymarkup or do INI files really not allow colons in the section headings. If that violates the INI file spec I'd be less happy about using it.
There was a problem hiding this comment.
Actions:
- I will add some inline docs
- I will check the INI file format to see if it allows
:in the section name
|
[Fixed] there is the extra [mariadb-centos7-atomicapp] in there |
rtnpro
force-pushed
the
refactor-config
branch
from
35c043d to
aea5161
Compare
rtnpro
force-pushed
the
refactor-config
branch
from
aea5161 to
fa4079d
Compare
|
rebase on latest master? |
rtnpro
force-pushed
the
refactor-config
branch
from
48cc336 to
fcb24d3
Compare
|
Another thing, we need to ensure that whatever delimiter we use for namespace, it should not appear in the names of the nulecule or graph item. So, chars like Also, the Nulecule spec does not speak about content of So, I think we can proceed with this merge as:
|
- Create a class representation for config data - Implement load() to load config data for a component, taking into account answers and user supplied data - Implement context() method to render flattened data to be consumed in a component artifact. - Improve API for config handling and wire up with Nulecule. - Allow comparing config instances for equality. - Added a placeholder to resolve provider config in Config. - Use ':' as namespace separator in answers and reference namespace separator from constants. - Use self.config if no config supplied in Nulecule load_config. - Store CLI data in Nulecule config. - During run/stop, reference config for provider. We are overriding provider data in config with cli provider, but when referencing provider info, we always do it from a single source of truth, config, in this case. - Update docs for Nulecule config. - Added helper in Config to retrieve resolved global params. - Centralize handling CLI answers in NuleculeManager. - process answers and cli_answers to create a nulecule.config.Config object - Include default provider, namespace in runtime answers, if missing. - Config takes default provider in spec into account. - Fixed unittests for refactored config.
- Create single source of truth for config data in NuleculeManager. Get rid of answers and cli_answers instance variables, and use only self.config to look up config data. - Allow ignoring sources when getting data from config. - Allow specifying file format when loading answers.
rtnpro
force-pushed
the
refactor-config
branch
from
fcb24d3 to
685c56a
Compare
|
Plan for merging this change:
|
|
hey @rtnpro I have reviewed this several times in the past. as far as this PR goes I am +1 if @cdrage approves. as for projectatomic/nulecule#211 shouldn't we also update https://github.com/projectatomic/atomicapp/blob/2f980660cb23ec3f5df53d06c4159d3af0390214/docs/spec/NULECULE_FILE.md within this repo? |
|
#dotests |
|
YAY! |
|
#dotests |
|
Local tests pass 👍 (waiting on CI now) |
|
k8s + docker pass, openshift timed out (tested it locally to make 100% it works) LGTM! thanks a lot @rtnpro ! |
Fixes #524