-
Notifications
You must be signed in to change notification settings - Fork 879
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
Add schema to apt configure config #357
Add schema to apt configure config #357
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
'properties': { | ||
'preserve_sources_list': { | ||
'type': 'boolean', | ||
'description': dedent("""\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)):
'description': dedent("""\ | |
'default': False, | |
'description': dedent("""\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
**mirror_property, | ||
'description': dedent("""\ | ||
The primary and security archive mirrors can | ||
be specified using the ``primary`` and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
be specified using the ``primary`` and | |
be specified using the ``primary`` and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
of configs, allowing mirrors to be specified | ||
on a per-architecture basis. Each config is a | ||
dictionary which must have an entry for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one more space indent for these lines
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indent one more (..,,etc throughout your schema description examples)
that config entry is for. The keyword | |
that config entry is for. The keyword |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
cloudinit/config/cc_apt_configure.py
Outdated
``updates`` => ``$RELEASE-updates``, | ||
``backports`` => ``$RELEASE-backports``. | ||
``security`` => ``$RELEASE-security``, | ||
``proposed`` => ``$RELEASE-proposed``, | ||
``release`` => ``$RELEASE``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
cloudinit/config/cc_apt_configure.py
Outdated
this template, the following strings will be | ||
replaced with the appropriate values: | ||
|
||
``$MIRROR``, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bullet points using a leading hyphen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 1
cloudinit/config/cc_apt_configure.py
Outdated
The ``source`` key supports variable | ||
replacements for the following strings: | ||
|
||
``$MIRROR``, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bullet points.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
cloudinit/config/cc_apt_configure.py
Outdated
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``""") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
cloudinit/config/cc_apt_configure.py
Outdated
}, | ||
'proxy': { | ||
'type': 'string', | ||
'description': dedent("""\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.',
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
cloudinit/config/cc_apt_configure.py
Outdated
Each entry under ``sources`` is a dictionary which | ||
may contain any of the following optional keys: | ||
|
||
``source``: a sources.list entry \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bullet points
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
70ff0b8
to
8af59d8
Compare
@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. |
f7c3eb9
to
d64fc87
Compare
@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 |
d64fc87
to
1ff2223
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good @lucasmoura! Couple minor nits and we are good to go.
cloudinit/config/cc_apt_configure.py
Outdated
'type': 'string', | ||
'description': dedent("""\ | ||
Specify configuration for apt, such as proxy | ||
configuratiun. This configuration is specified as a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
configuratiun. This configuration is specified as a | |
configuration. This configuration is specified as a |
cloudinit/config/cc_apt_configure.py
Outdated
'https_proxy': { | ||
'type': 'string', | ||
'description': dedent("""\ | ||
More convinient way to specify https apt proxy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More convinient way to specify https apt proxy. | |
More convenient way to specify https apt proxy. |
cloudinit/config/cc_apt_configure.py
Outdated
'http_proxy': { | ||
'type': 'string', | ||
'description': dedent("""\ | ||
More convinient way to specify http apt proxy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More convinient way to specify http apt proxy. | |
More convenient way to specify http apt proxy. |
@blackboxsw Done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work thanks @lucasmoura
This PR creates a schema object for the
apt_configure
module and validate this schema in thehandle
function of the module.There are some considerations regarding this PR:
primary
andsecurity
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 theReuse
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.debconf_selections
. I tried to infer what it supposed to do by looking at the code and thedebconf-set-selections
manpage, but my description may not be accurate or complete.lists
ornotes
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