schema: add json defs for modules U-Z by blackboxsw · Pull Request #1360 · canonical/cloud-init

blackboxsw · 2022-03-31T15:31:16Z

Proposed Commit Message

Migrate legacy schema to cloud-init-schema.json:
    - cc_ubuntu_advantage
    - cc_ubuntu_drivers
    - cc_write_files: added minItems: 1 to list to catch potential YAML typos
    - cc_write_files_deferred
    - cc_zypper_add_repo

Defined JSON schema for the following modules:
- cc_update_hostname
- cc_update_etc_hosts: Add DEPRECATED log for manage_etc_hosts: "template" as 
      it is non-intuitive alias of True value
- cc_yum_add_repo:  docstring incorrectly status PER_ALWAYS, but no module
  frequency attribute was defined, so cloud-init defaults to PER_INSTANCE.
  Set it PER_INSTANCE explicitly to avoid change in behavior.

Additional Context

Does not contain cc_user_groups as that's a fairly big module schema to sort, so I'd like to keep that module separate for
ease of review.

Test Steps

tox -re doc; xdg-open doc/rtd_html/topics/modules.html
PYTHONPATH=. python3 -m cloudinit.cmd.main devel schema --docs cc_update_hostname ....

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

TheRealFalcon

Left some comments inline. Additionally, see the following diff:

diff --git a/cloudinit/config/cc_zypper_add_repo.py b/cloudinit/config/cc_zypper_add_repo.py
index 330ef346..9b682bc6 100644
--- a/cloudinit/config/cc_zypper_add_repo.py
+++ b/cloudinit/config/cc_zypper_add_repo.py
@@ -3,7 +3,7 @@
 #
 # This file is part of cloud-init. See LICENSE file for license information.
 
-"""zypper_add_repo: Add zyper repositories to the system"""
+"""zypper_add_repo: Add zypper repositories to the system"""
 
 import os
 from textwrap import dedent
@@ -16,22 +16,25 @@ from cloudinit.config.schema import MetaSchema, get_meta_doc
 from cloudinit.settings import PER_ALWAYS
 
 distros = ["opensuse", "sles"]
-
+MODULE_DESCRIPTION = """\
+Zypper behavior can be configured using the ``config`` key, which will modify
+``/etc/zypp/zypp.conf``. The configuration writer will only append the
+provided configuration options to the configuration file. Any duplicate
+options will be resolved by the way the zypp.conf INI file is parsed.
+
+.. note::
+    Setting ``configdir`` is not supported and will be skipped.
+
+The ``repos`` key may be used to add repositories to the system. Beyond the
+required ``id`` and ``baseurl`` attributions, no validation is performed
+on the ``repos`` entries. It is assumed the user is familiar with the
+zypper repository file format.
+"""
 meta: MetaSchema = {
     "id": "cc_zypper_add_repo",
-    "name": "ZypperAddRepo",
+    "name": "Zypper Add Repo",
     "title": "Configure zypper behavior and add zypper repositories",
-    "description": dedent(
-        """\
-        Configure zypper behavior by modifying /etc/zypp/zypp.conf. The
-        configuration writer is "dumb" and will simply append the provided
-        configuration options to the configuration file. Option settings
-        that may be duplicate will be resolved by the way the zypp.conf file
-        is parsed. The file is in INI format.
-        Add repositories to the system. No validation is performed on the
-        repository file entries, it is assumed the user is familiar with
-        the zypper repository file format."""
-    ),
+    "description": MODULE_DESCRIPTION,
     "distros": distros,
     "examples": [
         dedent(
diff --git a/doc/rtd/topics/modules.rst b/doc/rtd/topics/modules.rst
index 093cee61..9541f675 100644
--- a/doc/rtd/topics/modules.rst
+++ b/doc/rtd/topics/modules.rst
@@ -64,4 +64,5 @@ Modules
 .. automodule:: cloudinit.config.cc_users_groups
 .. automodule:: cloudinit.config.cc_write_files
 .. automodule:: cloudinit.config.cc_yum_add_repo
+.. automodule:: cloudinit.config.cc_zypper_add_repo
 .. vi: textwidth=79

Changes:

Zypper module had a typo in its docstring
I decided to rewrite the zypper module description. It doesn't make much sense and doesn't follow inclusive naming standards.
Added the zypper module to the end of modules.rst so it gets included in the documentation.

TheRealFalcon · 2022-04-01T16:39:21Z

+      "properties": {
+        "manage_etc_hosts": {
+          "default": false,
+          "description": "Whether to manage /etc/hosts on the system. When set ``true``, cloud-init will render the hosts file using ``/etc/cloud/templates/hosts.tmpl`` replacing ``$hostname`` and ``$fdqn``. When set ``localhost``, Default: ``false``. DEPRECATED value ``template``will be dropped, use ``true`` instead.",


When set localhost, Default: false

^ I think you accidentally your sentence there

+1 I also changed the oneOf to "enum": [true, false, "template", "localhost"] which I think provides a better schema error message 'templatey' is not one of [True, False, 'template', 'localhost']`

versus
'templatey' is not valid under any of the given schemas

TheRealFalcon

One small language thing inline, but I trust you can fix that and merge when finished.

TheRealFalcon · 2022-04-08T03:27:08Z

          "default": false,
-          "description": "Whether to manage /etc/hosts on the system. When set ``true``, cloud-init will render the hosts file using ``/etc/cloud/templates/hosts.tmpl`` replacing ``$hostname`` and ``$fdqn``. When set ``localhost``, Default: ``false``. DEPRECATED value ``template``will be dropped, use ``true`` instead.",
-          "oneOf": [{"type": "boolean"}, {"enum": ["template", "localhost"]}]
+          "description": "Whether to manage ``/etc/hosts`` on the system. Set ``true`` to render the hosts file using ``/etc/cloud/templates/hosts.tmpl`` replacing ``$hostname`` and ``$fdqn``. Set ``localhost`` to and cloud-init will append a ``127.0.1.1`` entry that resolves from FQDN and hostname every boot. Default: ``false``. DEPRECATED value ``template`` will be dropped, use ``true`` instead.",


One more nit here: Set localhost to and cloud-init will append. Set localhost to what?

…ma.json

Add minItems: "1" to write_files schema to avoid empty list.

While schema still supports manage_etc_hosts: template, deprecate this value as it is an alias for true. Add DEPRECATED log for use of 'template' value.

frequency: ALWAYS was listed in the docstring, but it wasn't provided as a module-level freq attribute. So cloud-init actually defaults it to PER_INSTANCE. We don't want to change behavior, so let's set PER_INSTANCE here which also aligns with apt_configure module for setting up repos.

- cc_update_etc_hosts: drop "template" documentation - cc_update_hostname: missing dedents - cc_write_files: move distros= ["all"] into Meta drop schema comment - cc_yum: example for yum_repo_dir - cloud-init-schema.json: - manage_etc_hosts: descr ++ flat enum vs oneOf [boolean, enum] - yum: descr updates, patternPropery labels

blackboxsw force-pushed the schema-u-z branch from f9787c1 to 8da1d87 Compare April 1, 2022 16:21

TheRealFalcon suggested changes Apr 1, 2022

View reviewed changes

blackboxsw force-pushed the schema-u-z branch from 8da1d87 to a80aba4 Compare April 7, 2022 20:42

TheRealFalcon approved these changes Apr 8, 2022

View reviewed changes

blackboxsw added 11 commits April 8, 2022 13:30

schema: migrate ubuntu_advantage to static schema

ea09a0c

schema: migrate ubuntu_drivers to share status schema.json

1322b79

schema: migrate write_files_deferred to shared static cloud-init-sche…

c3a88d1

…ma.json

schema: migrate write_files to shared static cloud-init-schema.json

8b8d4af

Add minItems: "1" to write_files schema to avoid empty list.

schema: add update_etc_hosts static JSON schema

08120bd

While schema still supports manage_etc_hosts: template, deprecate this value as it is an alias for true. Add DEPRECATED log for use of 'template' value.

schema: add update_hostname static JSON schema

e639ad7

schema: migrate zypper_add_repo from legacy JSON schema definition

45f30cc

James patch suggestion

d656440

schema: change "Set localhost" to "If localhost, ..."

3c14180

blackboxsw force-pushed the schema-u-z branch from ad32ea9 to 3c14180 Compare April 8, 2022 19:33

blackboxsw merged commit dd87c03 into canonical:main Apr 8, 2022

This was referenced May 12, 2023

Top-level 'network' key results in error using v2 config #3815

Closed

Release 22.2 #3976

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

schema: add json defs for modules U-Z#1360

schema: add json defs for modules U-Z#1360
blackboxsw merged 11 commits into
canonical:mainfrom
blackboxsw:schema-u-z

blackboxsw commented Mar 31, 2022

Uh oh!

TheRealFalcon left a comment

Uh oh!

TheRealFalcon Apr 1, 2022

Uh oh!

blackboxsw Apr 7, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheRealFalcon left a comment

Uh oh!

TheRealFalcon Apr 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

blackboxsw commented Mar 31, 2022

Proposed Commit Message

Additional Context

Test Steps

Checklist:

Uh oh!

TheRealFalcon left a comment

Choose a reason for hiding this comment

Uh oh!

TheRealFalcon Apr 1, 2022

Choose a reason for hiding this comment

Uh oh!

blackboxsw Apr 7, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheRealFalcon left a comment

Choose a reason for hiding this comment

Uh oh!

TheRealFalcon Apr 8, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants