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

Update docs #5595

Merged
merged 4 commits into from
Aug 12, 2024
Merged

Update docs #5595

merged 4 commits into from
Aug 12, 2024

Conversation

holmanb
Copy link
Member

@holmanb holmanb commented Aug 8, 2024

Additional Context

Documents #5489, sprinkles in some more links to connect related pages, and moves the first boot determination docs into a dedicated page.

Merge type

  • Squash merge using "Proposed Commit Message"
  • Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

@holmanb holmanb added the wip Work in progress, do not land label Aug 8, 2024
Copy link
Collaborator

@a-dubs a-dubs left a comment

Choose a reason for hiding this comment

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

Looks great! One small typo. Thanks for updating docs! 💙

doc/rtd/explanation/boot.rst Show resolved Hide resolved
@holmanb holmanb force-pushed the holmanb/single-process-docs branch from 577fa19 to 6f6861f Compare August 8, 2024 22:11

- :command:`blame`
- :command:`show`
- :command:`dump`
- :command:`boot`

The analyze subcommand works by parsing the cloud-init log file for timestamps
Copy link
Member Author

Choose a reason for hiding this comment

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

This subcommand fails to capture interpreter startup time and library load time because of this fact, so at least advertise how it works so that users can draw their own conclusions.

@@ -65,7 +65,6 @@ Using ``instance-data``

``instance-data`` can be used in:

* :ref:`User data scripts<user_data_script>`.
Copy link
Member Author

Choose a reason for hiding this comment

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

This was redundant, since user data scripts are documented on the user data formats page.

@@ -2,18 +2,12 @@ Kernel command line
*******************

Providing configuration data via the kernel command line is somewhat of a last
resort, since this method only supports
:ref:`cloud config<user_data_formats-cloud_config>` starting with
Copy link
Member Author

Choose a reason for hiding this comment

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

This piece of information doesn't apply to datasource discovery override, and is already mentioned in the section below that it is relevant to. Drop it.

@holmanb
Copy link
Member Author

holmanb commented Aug 8, 2024

Looks great! One small typo. Thanks for updating docs! 💙

Thanks for the review @a-dubs. I just added some more content and fixed the issue you pointed out!

@holmanb holmanb removed the wip Work in progress, do not land label Aug 8, 2024
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.

I left a few comments inline, but nothing major.

cloudinit/cmd/main.py Show resolved Hide resolved
doc/rtd/explanation/first_boot.rst Show resolved Hide resolved
@@ -65,7 +65,6 @@ Using ``instance-data``

``instance-data`` can be used in:

* :ref:`User data scripts<user_data_script>`.
Copy link
Member

Choose a reason for hiding this comment

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

Why the removal? It's a valid usage

Copy link
Member Author

Choose a reason for hiding this comment

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

This was redundant, since user data scripts are documented on the user data formats page.

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha. As-is, it's a little weird though. The text says Cloud-config but links to the user data page (not your doing). We could update the text of the link to say 'user data' rather than 'cloud-config', you can only use instance data in cloud-config and user scripts, so I'm not sure we should be saying it can be used with user data when there are caveats to that.

I think we should either specify cloud-config and user data scripts independently and be ok with the redundancy, or we should link to the jinja format and say any user data format supported by jinja.

Copy link
Member Author

Choose a reason for hiding this comment

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

@TheRealFalcon good point. I just pushed an update, let me know what you think.

doc/rtd/howto/rerun_cloud_init.rst Show resolved Hide resolved
doc/rtd/reference/breaking_changes.rst Outdated Show resolved Hide resolved
@holmanb
Copy link
Member Author

holmanb commented Aug 9, 2024

@TheRealFalcon thanks for your review. I think that I've addressed all of your comments.

@blackboxsw blackboxsw added the documentation This Pull Request changes documentation label Aug 10, 2024
@holmanb
Copy link
Member Author

holmanb commented Aug 12, 2024

@TheRealFalcon ready for re-review, thanks!

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.

LGTM!

@holmanb holmanb force-pushed the holmanb/single-process-docs branch from 710c05e to 1c42f46 Compare August 12, 2024 16:15
@holmanb holmanb force-pushed the holmanb/single-process-docs branch from 1c42f46 to cadc6a9 Compare August 12, 2024 16:20
@holmanb
Copy link
Member Author

holmanb commented Aug 12, 2024

I just rebased the fixup commits and updated commit messages with PR number.

@holmanb holmanb merged commit baeb35c into canonical:main Aug 12, 2024
23 checks passed
holmanb added a commit that referenced this pull request Aug 12, 2024
Also shift the format page higher in the explanation page list, since
this is a high traffic page.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation This Pull Request changes documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants