Add schema to apt configure config by lucasmoura · Pull Request #357 · canonical/cloud-init

lucasmoura · 2020-05-07T14:06:16Z

This PR creates a schema object for the apt_configure module and validate this schema in the handle function of the module.

There are some considerations regarding this PR:

The primary and security keys have the exact same properties. I tried to eliminate this redundancy by moving their properties to a common place and then just referencing it for both security and primary. Similar to what is documented here under the Reuse paragraph. However, this approach does not work, because the # pointer goes to the beginning of the file, which is a python module instead of a json file, not allowing the pointer to find the correct definition. What I did was to create a separate dict for the mirror config and reuse it for primary and security, but maybe there are better approaches to do that.
There was no documentation for the config debconf_selections. I tried to infer what it supposed to do by looking at the code and the debconf-set-selections manpage, but my description may not be accurate or complete.
In the documentation generated, all property descriptions are bundled together in a single paragraph. This does not allows us to use lists or notes in the fields descriptions. This is generated because of this line of code, I tried to remove it to see how the docs would behave, but the result, was as expected, not good. But maybe there are other ways to include such markdown notations in the property description that I don't know about.

LP: #1858884

blackboxsw

Thanks for undertaking this @lucasmoura. It's a big one in that it exposes shortcomings in cloudinit/config/schema.py and gets you exercising tox -e doc vs. python3 -m cloudinit.cmd.main devel schema -d cc_apt_configure implementation.

We can chat about it a bit on Monday and come up with some thoughts on adapting schema.py to better handle structured whitespace in property descriptions.

If you have idea

blackboxsw · 2020-05-08T21:19:11Z

+            'properties': {
+                'preserve_sources_list': {
+                    'type': 'boolean',
+                    'description': dedent("""\


When we define the schema we should also advertize any default values with a default key too. The doc generations will add those defaults to documentation automatically at some point (they aren't added yet).

From the code below it looks like false if unprovided
if util.is_false(cfg.get('preserve_sources_list', False)):

Suggested change

'description': dedent("""\

'default': False,

'description': dedent("""\

blackboxsw · 2020-05-08T21:46:23Z

+                    **mirror_property,
+                    'description': dedent("""\
+                        The primary and security archive mirrors can
+                        be specified using the ``primary`` and


After the first line in a multi-line dedent, we'll need a single indent for all the remaining lines, otherwise the documentation text gets collapsed when rendering resulting in
"The primary and security archive mirrors canbe specified..."
you can validate this behavior with python3 -m cloudinit.cmd.main devel schema --docs cc_apt_configure and look at each line break in your text. some are fine, but in a few places we've missed that single space indent. I'll mark a couple below.

Suggested change

be specified using the ``primary`` and

be specified using the ``primary`` and

blackboxsw · 2020-05-08T21:47:00Z

+                        of configs, allowing mirrors to be specified
+                        on a per-architecture basis. Each config is a
+                        dictionary which must have an entry for


one more space indent for these lines

Suggested change

of configs, allowing mirrors to be specified

on a per-architecture basis. Each config is a

dictionary which must have an entry for

of configs, allowing mirrors to be specified

on a per-architecture basis. Each config is a

dictionary which must have an entry for

blackboxsw · 2020-05-08T21:48:00Z

+                        on a per-architecture basis. Each config is a
+                        dictionary which must have an entry for
+                         ``arches``, specifying which architectures
+                        that config entry is for. The keyword


indent one more (..,,etc throughout your schema description examples)

Suggested change

that config entry is for. The keyword

that config entry is for. The keyword

blackboxsw · 2020-05-08T22:12:41Z

+                             ``updates`` => ``$RELEASE-updates``,
+                             ``backports`` => ``$RELEASE-backports``.
+                             ``security`` => ``$RELEASE-security``,
+                             ``proposed`` => ``$RELEASE-proposed``,
+                             ``release`` => ``$RELEASE``.


Each of these lines in sphinx minimally needs a leading hyphen '- ' to ensure that they are rendered in readthedocs as bullet items in a list
See https://cloudinit.readthedocs.io/en/latest/topics/modules.html#apt-configure "disable apt sources"
versus your local rendered docs.
To render your docs locally to test and view the differences:
tox -e doc # which generates docs/rtd_html/index.html
you can browse locally with your browser of choice pointing at the URL like:
file:///home//src/cloud-init/doc/rtd_html/topics/modules.html#apt-configure

Additionally, something in the schema rendering is damaging the multi-line white space that should exist between

``disable_suites``: - ``updates`` => ``$RELEASE-updates``,

So, we'll have to track that down because all this text is being ultimately rendered as a single wrapped line instead of separate bullet points.

blackboxsw · 2020-05-08T22:39:24Z

+                        this template, the following strings will be
+                        replaced with the appropriate values:
+
+                             ``$MIRROR``,


bullet points using a leading hyphen

blackboxsw · 2020-05-08T22:39:55Z

+                         The ``source`` key supports variable
+                         replacements for the following strings:
+
+                             ``$MIRROR``,


bullet points.

blackboxsw · 2020-05-08T22:47:09Z

+                         regex in ``add_apt_repo_match`` will be added to
+                         the system using ``add-apt-repository``. If
+                          ``add_apt_repo_match`` is not specified, it
+                         defaults to ``^[\\w-]+:\\w``""")


Please use the global ADD_APT_REPO_MATCH from the file instead of repeating in docs (in case it changes)

It's probably worth adding a 'default': ADD_APT_REPO_MATCH, here too to the schema

blackboxsw · 2020-05-08T22:55:08Z

+                },
+                'proxy': {
+                    'type': 'string',
+                    'description': dedent("""\


I don't think you need the dedent here we just use it when multiple lines of text were needed.
the below fits in 80 chars
'description': 'Alias for defining a http apt proxy.',

blackboxsw · 2020-05-08T22:56:32Z

+                         Each entry under ``sources`` is a dictionary which
+                         may contain any of the following optional keys:
+
+                             ``source``: a sources.list entry \


bullet points

lucasmoura · 2020-05-12T13:16:37Z

@blackboxsw I have addressed the issues you raised. We just need to figure out how to proceed with the description problem. If you believe we can address it in this PR, I can start working on it without an issue.

lucasmoura · 2020-05-13T13:23:07Z

@blackboxsw I think I have solved the description parsing problem. But if you think there is a better solution, no problem, I can work on it as well

blackboxsw

Looks really good @lucasmoura! Couple minor nits and we are good to go.

blackboxsw · 2020-05-13T16:56:31Z

+                    'type': 'string',
+                    'description':  dedent("""\
+                        Specify configuration for apt, such as proxy
+                        configuratiun. This configuration is specified as a


Suggested change

configuratiun. This configuration is specified as a

configuration. This configuration is specified as a

blackboxsw · 2020-05-13T16:57:18Z

+                'https_proxy': {
+                    'type': 'string',
+                    'description': dedent("""\
+                        More convinient way to specify https apt proxy.


Suggested change

More convinient way to specify https apt proxy.

More convenient way to specify https apt proxy.

blackboxsw · 2020-05-13T16:57:31Z

+                'http_proxy': {
+                    'type': 'string',
+                    'description': dedent("""\
+                        More convinient way to specify http apt proxy.


Suggested change

More convinient way to specify http apt proxy.

More convenient way to specify http apt proxy.

lucasmoura · 2020-05-13T19:48:47Z

@blackboxsw Done

blackboxsw

Excellent work thanks @lucasmoura

blackboxsw self-assigned this May 7, 2020

blackboxsw requested changes May 8, 2020

View reviewed changes

lucasmoura force-pushed the add-schema-to-apt-configure-config branch from 70ff0b8 to 8af59d8 Compare May 12, 2020 13:01

Lucas Moura added 11 commits May 13, 2020 10:18

Add initial version of apt_configure schema

99faf21

Better format schema for documentation

a201213

Validate schema in apt configure handle

e6e0a6c

Update debconf_selections in cloud-config example

b5986aa

Keep apt_configure title as it is

c8d8dcc

Better format schema

915685a

Fix apt schema examples

7b59660

Add default variable to apt schema

87e59bc

Use ADD_APT_REPO_MATCH in apt_configure schema

e6539ab

Add bullet points on apt_configure schema

c3b6ae2

Better format bullet points

83584a2

lucasmoura force-pushed the add-schema-to-apt-configure-config branch from f7c3eb9 to d64fc87 Compare May 13, 2020 13:21

Update description parser

1ff2223

lucasmoura force-pushed the add-schema-to-apt-configure-config branch from d64fc87 to 1ff2223 Compare May 13, 2020 19:06

blackboxsw requested changes May 13, 2020

View reviewed changes

Fix typos in cc_apt_configure schema

6c4513a

blackboxsw approved these changes May 13, 2020

View reviewed changes

blackboxsw merged commit 2e32c40 into canonical:master May 13, 2020

lucasmoura mentioned this pull request Jun 9, 2020

Apt configure yaml test cloud-init/ubuntu-sru#108

Merged

ubuntu-server-builder mentioned this pull request May 12, 2023

Release 20.3 #3774

Closed

	'description': dedent("""\
	'default': False,
	'description': dedent("""\

	be specified using the ``primary`` and
	be specified using the ``primary`` and

	that config entry is for. The keyword
	that config entry is for. The keyword

	configuratiun. This configuration is specified as a
	configuration. This configuration is specified as a

	More convinient way to specify https apt proxy.
	More convenient way to specify https apt proxy.

	More convinient way to specify http apt proxy.
	More convenient way to specify http apt proxy.

Conversation

lucasmoura commented May 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blackboxsw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lucasmoura commented May 12, 2020

Uh oh!

lucasmoura commented May 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blackboxsw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lucasmoura commented May 13, 2020

Uh oh!

blackboxsw left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lucasmoura commented May 7, 2020 •

edited

Loading

lucasmoura commented May 13, 2020 •

edited

Loading