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

doc(nocloud): Document network-config file #5204

Merged
merged 6 commits into from
May 29, 2024

Conversation

holmanb
Copy link
Member

@holmanb holmanb commented Apr 23, 2024

Additional Context

Based on #5193 (review)

@holmanb holmanb force-pushed the holmanb/ftp-docs-2 branch 2 times, most recently from 1cce79b to 5a623dd Compare April 23, 2024 20:22
Copy link
Collaborator

@blackboxsw blackboxsw left a comment

Choose a reason for hiding this comment

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

LGTM, couple of nits.

doc/rtd/reference/datasources/nocloud.rst Outdated Show resolved Hide resolved
doc/rtd/reference/datasources/nocloud.rst Show resolved Hide resolved
doc/rtd/reference/datasources/nocloud.rst Outdated Show resolved Hide resolved
doc/rtd/reference/datasources/nocloud.rst Outdated Show resolved Hide resolved
@holmanb holmanb requested a review from blackboxsw April 24, 2024 16:43
@holmanb
Copy link
Member Author

holmanb commented Apr 24, 2024

@blackboxsw I think that I've addressed your comments.

Copy link
Collaborator

@blackboxsw blackboxsw left a comment

Choose a reason for hiding this comment

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

LGTM, minor typo and a question about command line vs commandline.

.. note::

The aliases ``s`` , ``h`` and ``i`` are only supported by kernel
commandline or SMBIOS. When configured in a ``*.cfg`` file, the long key
Copy link
Collaborator

Choose a reason for hiding this comment

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

Non-blocking comment: I think it's "command line" not commandline. But I see we are mixed throughout our docs 15 count of commandline 25 count of "command line".

Suggested change
commandline or SMBIOS. When configured in a ``*.cfg`` file, the long key
command line or SMBIOS. When configured in a ``*.cfg`` file, the long key

Copy link
Member Author

Choose a reason for hiding this comment

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

will update the 15 incorrect instances of commandline

doc/rtd/reference/datasources/nocloud.rst Outdated Show resolved Hide resolved
@holmanb
Copy link
Member Author

holmanb commented Apr 25, 2024

@blackboxsw I've updated the docs, docstrings comments in cloud-init, and user-facing text with the correct spelling. Some file names and function names remain incorrect, but fixing those would be more invasive so for now they stay.

@holmanb holmanb requested a review from blackboxsw April 25, 2024 15:16
@@ -19,7 +19,7 @@ knowledge and become better at using and configuring ``cloud-init``.
vendordata.rst
security.rst
analyze.rst
kernel-cmdline.rst
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's avoid doc file renaming throughout unless we have a really compelling reason This causes the generated docs generated to be new html pages and routes and we'd also need to setup redirect rules in RTD for each of these changes in the event of external links in blog posts etc.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry this comment of mine was arbitrary and we should do what is right and more clear for end users without concern for any extra redirects we may need to setup to facilitate better naming of docs and files. kernel_command_line is much more clear a slug than kernel_cmdline for future doc readers, so let's just do it. I've reverted your latest commit which changed just this and added the necessary redirect configuration in readthedocs.org for this redirect forwarding from kernel-cmdline.html -> kernel-command-line.html. /Me backs down on this perspective and just fixes this branch as you had it.

Copy link
Collaborator

@blackboxsw blackboxsw left a comment

Choose a reason for hiding this comment

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

Changes look good thanks, let's just avoid any top-level docs file renames to avoid thrashing with potential external links to cloud-init docs.

Copy link
Collaborator

@blackboxsw blackboxsw left a comment

Choose a reason for hiding this comment

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

Let's avoid rst doc file renames, otherwise +1.

Copy link

Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close.

If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging TheRealFalcon, and he will ensure that someone takes a look soon.

(If the pull request is closed and you would like to continue working on it, please do tag TheRealFalcon to reopen it.)

@github-actions github-actions bot added the stale-pr Pull request is stale; will be auto-closed soon label May 18, 2024
@github-actions github-actions bot closed this May 26, 2024
@holmanb holmanb reopened this May 26, 2024
@holmanb holmanb removed the stale-pr Pull request is stale; will be auto-closed soon label May 26, 2024
Copy link
Collaborator

@blackboxsw blackboxsw left a comment

Choose a reason for hiding this comment

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

LGTM. I've reverted your last commit based on the fact that I disagree with my former review comment. We should move toward the best represented document names/slugs/urls we can and sort historic links/aliases and redirects appropriately in RTD configuration. Eventually be may run out of redirects on the project for renames as I think we may be capped at 100 redirects on the proejct, but it makes little sense to avoid common sense URL/slug/page renames which improve discoverability an use common terminology throughout cloud-init.

@@ -19,7 +19,7 @@ knowledge and become better at using and configuring ``cloud-init``.
vendordata.rst
security.rst
analyze.rst
kernel-cmdline.rst
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry this comment of mine was arbitrary and we should do what is right and more clear for end users without concern for any extra redirects we may need to setup to facilitate better naming of docs and files. kernel_command_line is much more clear a slug than kernel_cmdline for future doc readers, so let's just do it. I've reverted your latest commit which changed just this and added the necessary redirect configuration in readthedocs.org for this redirect forwarding from kernel-cmdline.html -> kernel-command-line.html. /Me backs down on this perspective and just fixes this branch as you had it.

@@ -1,4 +1,4 @@
.. _kernel_cmdline:
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can drop this now given we autolabel sections perform section autolabels. This means instead our cross references will be :ref:'explanation/kernel-command-line:Kernel command line' instead of adding anchors where we have sections throughout code

@holmanb holmanb merged commit fa1b868 into canonical:main May 29, 2024
28 of 29 checks passed
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.

None yet

2 participants