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

Add LXD and OpenStack image documentation #289

Merged
merged 2 commits into from
Nov 8, 2024

Conversation

mstpn
Copy link
Member

@mstpn mstpn commented Oct 23, 2024

Our public docs were missing explanations for our image offerings on cloud-images.ubuntu.com for LXD and OpenStack.

An "Explanation" index and landing page has been created on the public-images project now that there is more than one Explanation article.

A commit fixing the American spelling of artifact to the British spelling artefact across our docs is included in this PR as well.

Artefact(s) is the British spelling and should be used. `artifacts` was
in the `custom_wordlist` and was hiding this.
Copy link
Contributor

@codyshepherd codyshepherd left a comment

Choose a reason for hiding this comment

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

A couple of minor things, but otherwise lgtm.

public-images/public-images-explanation/index.rst Outdated Show resolved Hide resolved
@mstpn mstpn force-pushed the cpc3365-lxd-and-openstack-images branch from 0d54316 to 6bd4be9 Compare November 4, 2024 20:29
Copy link
Collaborator

@k-dimple k-dimple left a comment

Choose a reason for hiding this comment

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

Thanks Matthew, this is really good. My comments are mostly just nits.

public-images/index.rst Outdated Show resolved Hide resolved

- The bootable disk image for a virtual machine is a :ref:`qcow-ref` image (``*.img``)

The following are example commands to import LXD containers and virtual machines, respectively, based on downloaded Ubuntu 24.04 artefacts:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
The following are example commands to import LXD containers and virtual machines, respectively, based on downloaded Ubuntu 24.04 artefacts:
The following are example commands to import an image into LXD containers and virtual machines, respectively, based on downloaded Ubuntu 24.04 artefacts:

Does that make sense? Or maybe something like 'to import an image to create LXD containers and virtual machines'?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch here, I think some context got lost when I was reworking the sentence. This makes sense to me: "The following are example commands to import an image for creating LXD containers and virtual machines based on downloaded Ubuntu 24.04 artifacts."

Copy link
Collaborator

Choose a reason for hiding this comment

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

You could have directly included that! :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought I had :) Wrote both down in the editor to compare and must have deleted the new one instead of the old.

— which exposes the full functionality of the ``guestfs`` API — or
rely on the ``virt-*`` tools from ``libguestfs`` — like ``virt-cat`` for
displaying files, ``virt-df`` for checking free space, and
``virt-inspector`` for inspecting VM images — to perform specific tasks.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
``virt-inspector`` for inspecting VM images — to perform specific tasks.
``virt-inspector`` for inspecting VM images.

Copy link
Collaborator

@k-dimple k-dimple Nov 7, 2024

Choose a reason for hiding this comment

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

Looks like you missed this change. Isn't it easier to add all the suggestions that you agree with into a single batch commit from the git UI itself (rather than changing all of them again in your version)?

Copy link
Member Author

@mstpn mstpn Nov 8, 2024

Choose a reason for hiding this comment

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

Whoops, many changes to this sentence and this one slipped.

I usually fix in one commit and then rebase to keep the commit history clean, and haven't looked into doing that via the GitHub UI. And I run the spell/link checks locally as well after changes instead of hoping they pass CI.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Using the Add suggestion to batch in the UI option allows you to create a single commit. But yeah you'll still need to squash commits to keep the commit history clean and run the spell/link checks locally.

@mstpn mstpn force-pushed the cpc3365-lxd-and-openstack-images branch from 6bd4be9 to ef547d1 Compare November 7, 2024 01:11
@mstpn
Copy link
Member Author

mstpn commented Nov 7, 2024

Thanks for the detailed review as always @k-dimple.

@mstpn mstpn requested a review from k-dimple November 7, 2024 01:13
Copy link
Collaborator

@k-dimple k-dimple left a comment

Choose a reason for hiding this comment

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

LGTM. Just add your version of the statement (the one that made sense to you) and remove the last bit that was missed.

Thanks for creating all the links!

— which exposes the full functionality of the ``guestfs`` API — or
rely on the ``virt-*`` tools from ``libguestfs`` — like ``virt-cat`` for
displaying files, ``virt-df`` for checking free space, and
``virt-inspector`` for inspecting VM images — to perform specific tasks.
Copy link
Collaborator

@k-dimple k-dimple Nov 7, 2024

Choose a reason for hiding this comment

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

Looks like you missed this change. Isn't it easier to add all the suggestions that you agree with into a single batch commit from the git UI itself (rather than changing all of them again in your version)?

@mstpn mstpn force-pushed the cpc3365-lxd-and-openstack-images branch from ef547d1 to 51fe930 Compare November 8, 2024 02:01
@k-dimple k-dimple merged commit 87d8fdd into canonical:main Nov 8, 2024
9 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.

3 participants