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 support for qcow2 as an image format. #1425

Merged
merged 1 commit into from
Apr 1, 2021

Conversation

mikalstill
Copy link
Contributor

Issue number:

None, should every PR have an associated issue? CONTRIBUTING.md appears ambigious on this point.

Description of changes:

Add support for qcow2 as an output format for iamges. qcow2 is pretty common in terms of image formats. Its what OpenStack, KVM, and so on use as their VM image format. This patch adds support
for creating images in that format.

Testing done:

I have verified the output images by booting them on a KVM system.

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

@tjkirch tjkirch requested a review from bcressey March 29, 2021 21:45
tools/rpm2img Outdated
Comment on lines 236 to 237
chown 1000:1000 "${OUTPUT_DIR}/${DISK_IMAGE_BASENAME}.${IMAGE_EXTENSION}" \
"${OUTPUT_DIR}/${DATA_IMAGE_BASENAME}.${IMAGE_EXTENSION}"
Copy link
Contributor

Choose a reason for hiding this comment

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

We can just chown everything under ${OUTPUT_DIR} - then we don't need to keep track of IMAGE_EXTENSION, and we only need to run it once at the very end.

Suggested change
chown 1000:1000 "${OUTPUT_DIR}/${DISK_IMAGE_BASENAME}.${IMAGE_EXTENSION}" \
"${OUTPUT_DIR}/${DATA_IMAGE_BASENAME}.${IMAGE_EXTENSION}"
find "${OUTPUT_DIR}" -type f -print -exec chown 1000:1000 {} \;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

tools/rpm2img Show resolved Hide resolved
@mikalstill
Copy link
Contributor Author

Thanks for the comments. I was going for the minimum change, I'll take a look at the proposed comments tomorrow and incorporate them.

@webern webern self-requested a review March 30, 2021 18:52
fi

find "${OUTPUT_DIR}" -type f -print -exec chown 1000:1000 {} \;

lz4 -9vc "${BOOT_IMAGE}" >"${OUTPUT_DIR}/${BOOT_IMAGE_NAME}"
lz4 -9vc "${VERITY_IMAGE}" >"${OUTPUT_DIR}/${VERITY_IMAGE_NAME}"
lz4 -9vc "${ROOT_IMAGE}" >"${OUTPUT_DIR}/${ROOT_IMAGE_NAME}"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: we can move the find command below these lz4 steps, and delete the other chown command since it's redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@bcressey
Copy link
Contributor

LGTM apart from the nitpick. Can you squash your two commits together before we merge?

qcow2 is pretty common in terms of image formats. Its what OpenStack,
KVM, and so on use as their VM image format. This patch adds support
for creating images in that format. I have verified the output images
by booting them on a KVM system.
Copy link
Contributor

@bcressey bcressey 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, thank you!

Copy link
Contributor

@zmrow zmrow left a comment

Choose a reason for hiding this comment

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

🍰

@zmrow
Copy link
Contributor

zmrow commented Mar 31, 2021

Thanks again for the contribution! Nice work.

Just for future reference and since you had a question about it: CONTRIBUTING.md mentions opening an issue before a PR. This helps us discuss the changes before you start working on them, and avoids any wasted work if we're already working on it!

You open an issue first to discuss any significant work - we would hate for your time to be wasted.

@bcressey bcressey merged commit fc84271 into bottlerocket-os:develop Apr 1, 2021
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.

5 participants