-
Notifications
You must be signed in to change notification settings - Fork 666
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Extend schema validation #3765
Extend schema validation #3765
Conversation
c779bf9
to
7d2db25
Compare
Stupid question (and possibly bad idea ?): While we're at "patching" the schema file to add the one from drivers, can't we generate the list of valid driver name at run time ? This would allow the linter to complain that someone is trying to use an unknown driver. Unknown as in "not installed" |
Thanks for your review. Good question. IMHO, the order of operations is installing driver first and then setting driver name in With Let me know if you have concern. It seems this feature existed in 4.0.1 and was removed in 4.0.2. 4.0.1❯ pip freeze | grep molecule
molecule==4.0.1
molecule-docker==1.1.0
❯ molecule list
---
dependency:
name: galaxy
driver:
name: podman
platforms:
- image: quay.io/centos/centos:stream8
name: instance
pre_build_image: true
provisioner:
name: ansible
verifier:
name: ansible
CRITICAL Failed to pre-validate.
{'driver': [{'name': ['unallowed value podman']}]} 4.0.2❯ pip freeze | grep molecule
molecule==4.0.2
molecule-docker==1.1.0
❯ molecule list
Traceback (most recent call last):
File "/Users/jackzhang/.pyenv/versions/v-397/bin/molecule", line 8, in <module>
sys.exit(main())
File "/Users/jackzhang/.pyenv/versions/3.9.7/envs/v-397/lib/python3.9/site-packages/click/core.py", line 1130, in __call__
return self.main(*args, **kwargs)
File "/Users/jackzhang/.pyenv/versions/3.9.7/envs/v-397/lib/python3.9/site-packages/click/core.py", line 1055, in main
rv = self.invoke(ctx)
File "/Users/jackzhang/.pyenv/versions/3.9.7/envs/v-397/lib/python3.9/site-packages/click/core.py", line 1657, in invoke
return _process_result(sub_ctx.command.invoke(sub_ctx))
File "/Users/jackzhang/.pyenv/versions/3.9.7/envs/v-397/lib/python3.9/site-packages/click/core.py", line 1404, in invoke
return ctx.invoke(self.callback, **ctx.params)
File "/Users/jackzhang/.pyenv/versions/3.9.7/envs/v-397/lib/python3.9/site-packages/click/core.py", line 760, in invoke
return __callback(*args, **kwargs)
File "/Users/jackzhang/.pyenv/versions/3.9.7/envs/v-397/lib/python3.9/site-packages/click/decorators.py", line 26, in new_func
return f(get_current_context(), *args, **kwargs)
File "/Users/jackzhang/.pyenv/versions/3.9.7/envs/v-397/lib/python3.9/site-packages/molecule/command/list.py", line 114, in list
s = scenarios.Scenarios(base.get_configs(args, command_args), scenario_name)
File "/Users/jackzhang/.pyenv/versions/3.9.7/envs/v-397/lib/python3.9/site-packages/molecule/command/base.py", line 189, in get_configs
configs = [
File "/Users/jackzhang/.pyenv/versions/3.9.7/envs/v-397/lib/python3.9/site-packages/molecule/command/base.py", line 190, in <listcomp>
config.Config(
File "/Users/jackzhang/.pyenv/versions/3.9.7/envs/v-397/lib/python3.9/site-packages/molecule/config.py", line 64, in __call__
obj.after_init()
File "/Users/jackzhang/.pyenv/versions/3.9.7/envs/v-397/lib/python3.9/site-packages/molecule/config.py", line 112, in after_init
self.config = self._reget_config()
File "/Users/jackzhang/.pyenv/versions/3.9.7/envs/v-397/lib/python3.9/site-packages/molecule/config.py", line 284, in _reget_config
env = util.merge_dicts(os.environ, self.env)
File "/Users/jackzhang/.pyenv/versions/3.9.7/envs/v-397/lib/python3.9/site-packages/molecule/config.py", line 197, in env
"MOLECULE_INSTANCE_CONFIG": self.driver.instance_config,
File "/Users/jackzhang/.pyenv/versions/3.9.7/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/functools.py", line 969, in __get__
val = self.func(instance)
File "/Users/jackzhang/.pyenv/versions/3.9.7/envs/v-397/lib/python3.9/site-packages/molecule/config.py", line 181, in driver
driver = api.drivers(config=self)[driver_name]
File "/Users/jackzhang/.pyenv/versions/3.9.7/envs/v-397/lib/python3.9/site-packages/molecule/api.py", line 29, in __getitem__
return self.__dict__[i]
KeyError: 'podman' |
@zhan9san First thing is fixing the issues with pre-commit need to be resolved. IMHO fix the pipeline first, then I'll check the code and review. Thanks for your effort on this, I appreciate the effort to fix this. |
lint is still failing |
Only one schema file is accepted in current schema test, I'll try to figure it out. It would be great if you could give me some advice on the implementation in this PR. I am afraid if if I take extra time to fix the test/lint issue, but finally, you reject this proposal. Appreciate your time. |
7d2db25
to
c1e7a29
Compare
7392363
to
8dc674c
Compare
@zhan9san Can you please rebase this? |
Sure, I'll do it tomorrow |
8dc674c
to
0deb261
Compare
0b31973
to
080548c
Compare
080548c
to
898defd
Compare
@ssbarnea Could you help review it? |
Sorry, I've not had time recently (and I've been too upset since 4.0.2 to focus), but I am looking at this now with the most recent version of molecule-proxmox driver (which we just updated to support proxmox's cloud-init features). Many thanks. [edit: since 4.0.2] |
@zhan9san This assumes that driver schema is used only by molecule itself, which is not true. Removing all these test files makes me think that it might be too each to break the schema. |
@ssbarnea Thanks for your attention. If the driver schema is defined in molecule repo, each time the molecule-plugins repo has schema update, it has to make related change in molecule repo as well, which is not convenient to maintain the schema in another repo. If we don't do it in this way, could you give us some suggestion to support customized plugins? The removed test files are redundant because check-jsonschema has done the same thing. As removed test files for drivers, if we move schema files outside the molecule, the test files need to be moved as well, that's why they are removed. If it is not used for molecule only, where is it used? Would it be possible to provide more details? And how is it affected? |
@zhan9san Sorry for not commenting. While I agree with the aim, I'm not sure how it can be done, my knowledge in json schema handling is limited to updating/fixing it. So I'm not sure I can review it properly. Also, Sorin need to review this PR anyway as he knows how to check that others users of the schema are still fine. I guess it's mainly vscode stuff and I fear that vscode schema support will impact this PR quite a lot. |
Thanks for all the same. If all driver schema are combined into one schema file, I am afraid the only one schema will grow too large to maintain. |
Signed-off-by: Jack Zhang <jack4zhang@gmail.com>
898defd
to
7333ad2
Compare
Hey @zhan9san ! Not sure if is the correct place to ask, in case is not let me know.
I see the driver, but when running I'd appreciate if you could help me, and thanks in advance! |
Hi @Oscar4596, you might wanna open a discussion item, or, if you think you found a bug, an issue with your findings. |
Signed-off-by: Jack Zhang jack4zhang@gmail.com
With this PR, the schema can be defined in driver itself. And it make some private driver work as well.
Related to #3689 and #3638 (comment)
@dmlb2000 @ssbarnea @meffie
Could you help review it?