Skip to content
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

Merged
merged 1 commit into from Apr 19, 2023
Merged

Conversation

zhan9san
Copy link
Contributor

@zhan9san zhan9san commented Dec 2, 2022

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?

@apatard
Copy link
Contributor

apatard commented Dec 2, 2022

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"

@zhan9san
Copy link
Contributor Author

zhan9san commented Dec 2, 2022

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"

@apatard

Thanks for your review.

Good question. IMHO, the order of operations is installing driver first and then setting driver name in molecule.yml.

With allow the linter to complain, it will have a friendly UI. We can do this in another PR as I noticed that the latest version 4.0.3 didn't support this either.

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'

@dmlb2000
Copy link
Contributor

dmlb2000 commented Dec 2, 2022

@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.

@ssbarnea
Copy link
Member

ssbarnea commented Dec 3, 2022

lint is still failing

@zhan9san
Copy link
Contributor Author

zhan9san commented Dec 4, 2022

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.

@zhan9san zhan9san marked this pull request as draft December 5, 2022 10:24
@zhan9san zhan9san force-pushed the feature/extend-schema branch 3 times, most recently from 7392363 to 8dc674c Compare December 6, 2022 08:00
@zhan9san zhan9san marked this pull request as ready for review December 6, 2022 08:08
@zhan9san
Copy link
Contributor Author

zhan9san commented Dec 6, 2022

lint is still failing

@ssbarnea
The schema test written in TypeScript is refactored in check-jsonschema in tox->pytest->test_schema.py.

@dmlb2000
Could you help review this PR?

Let me know if you have any concern.

@ssbarnea
Copy link
Member

@zhan9san Can you please rebase this?

@zhan9san
Copy link
Contributor Author

Sure, I'll do it tomorrow

@zhan9san
Copy link
Contributor Author

@ssbarnea Could you help review it?

@zhan9san
Copy link
Contributor Author

@ssbarnea @apatard

Do you have concerns about this PR?

I'd like to hear your suggestions.

@meffie
Copy link

meffie commented Apr 18, 2023

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]

@ssbarnea
Copy link
Member

@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.

@zhan9san
Copy link
Contributor Author

zhan9san commented Apr 18, 2023

@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?

@apatard
Copy link
Contributor

apatard commented Apr 19, 2023

@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.

@zhan9san
Copy link
Contributor Author

@apatard

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>
@Oscar4596
Copy link

Hey @zhan9san ! Not sure if is the correct place to ask, in case is not let me know.
I'm getting a "driver is not installed" error when trying to use a custom driver for molecule.
When I run:

$ molecule drivers
                                                                                
──────────────────────╴
  custom_ec2_bundled                                                            
  delegated                                                                     
  ec2    

I see the driver, but when running molecule test or even molecule test -d custom_ec2_bundled I still get the error. I think I might be missing something in the custom driver configuration.
I install the driver using pip from a private repo.
The validation for a driver is made in this line.

I'd appreciate if you could help me, and thanks in advance!

@ziegenberg
Copy link
Contributor

Hi @Oscar4596, you might wanna open a discussion item, or, if you think you found a bug, an issue with your findings.
This is a closed Pull Request most probably not the right place to ask.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants