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

schema: add json for modules K-L #1321

Merged
merged 7 commits into from
Mar 28, 2022
Merged

Conversation

blackboxsw
Copy link
Collaborator

@blackboxsw blackboxsw commented Mar 8, 2022

Proposed Commit Message

schema: add JSON defs for modules K-L

Includes:
- cc_keyboard: migrated legacy scheme to cloud-init-schema.json
- cc_keys_to_console:
- cc_landscape: added schema defs for most frequent client keys
- cc_locale
- cc_lxd

Dropping duplicates docs as schema examples now cover them
 - doc/examples/cloud-config-landscape.txt 
 - doc/examples/cloud-config-lxd.txt 

LP: #1858899, #1858900, #1858901, #1858902

Additional Context

Test Steps

tox -e py3 tests/unittests/config
tox -e doc
xdg-open doc/rtd_html/topics/modules.html
for mod in cc_locale cc_lxd cc_landscape cc_keyboard cc_keys_to_console; do
    PYTHONPATH=. python3 -m cloudinit.cmd.main devel schema --docs \$mod;
done

PYTHONPATH=. python3 -m cloudinit.cmd.main devel schema --docs cc_landscape

Checklist:

  • My code follows the process laid out in the documentation
  • I have updated or added any unit tests accordingly
  • I have updated or added any documentation accordingly

Copy link
Member

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

Overall looks good. I left some comments inline. Additionally,

For landscape, we might want a way to indicate that the schema params are optional. Doesn't necessarily need to be inline in every parameter, but it's probably worth mentioning in the module description.

General comment: When setting distros or frequency in the meta schema, I don't think we need a separate variable definition if that's the only place it's used. E.g., use "distros": ["all"], and "frequency": PER_INSTANCE, instead of using new variables that don't get used anywhere else.

Rebase will be needed too.

cloudinit/config/cc_keys_to_console.py Outdated Show resolved Hide resolved
cloudinit/config/cloud-init-schema.json Outdated Show resolved Hide resolved
cloudinit/config/cloud-init-schema.json Outdated Show resolved Hide resolved
cloudinit/config/cc_landscape.py Outdated Show resolved Hide resolved
cloudinit/config/cloud-init-schema.json Outdated Show resolved Hide resolved
tests/unittests/config/test_cc_landscape.py Show resolved Hide resolved
cloudinit/config/cc_lxd.py Outdated Show resolved Hide resolved
@blackboxsw blackboxsw force-pushed the schema-k-l branch 2 times, most recently from 1c73d5b to 3033124 Compare March 25, 2022 22:24
Move definition into cloud-init-schema.json
@blackboxsw
Copy link
Collaborator Author

General comment: When setting distros or frequency in the meta schema, I don't think we need a separate variable definition if that's the only place it's used. E.g., use "distros": ["all"], and "frequency": PER_INSTANCE, instead of using new variables that don't get used anywhere else.

Turns out we do need them (see CI failures :)) because cloudinit.stages and cloudinit.config.fixup_module looks for module.frequency and module.distros attributes directly, which I think we need to fix to instead look for module.meta[]. I've added a separate jira card #888 for this. We can drop the module.frequency attribute from our schema definitions if the frequency is PER_INSTANCE as fixup_module will properly set the default when "absent" to PER_INSTANCE anyway.

Definition cc_keys_to_console schema in into cloud-init-schema.json
Move definition into cloud-init-schema.json

Add test that get_schema parses packaged cloud-init-schema.json.
Definition cc_lxd schema in into cloud-init-schema.json
Definition cc_landscape schema in into cloud-init-schema.json
Supplement schema defs with known typical landscape config options.
@TheRealFalcon
Copy link
Member

Ooph, that's bad. We should add a test (not part of this PR) that we're not getting that warning anywhere. I may have changed that in previous modules such that modules are now running per-instance rather than what they're being declared to run as.

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

Successfully merging this pull request may close these issues.

2 participants