Skip to content

Conversation

cgwalters
Copy link
Collaborator

This parallels what we do with ephemeral, where it's super
natural because containers have labels.

With libvirt we already have custom stuff in the XML, so
add support for reading/writing labels there, and filtering
by them in bcvk libvirt list --label=foo etc.

This makes it easier to use bcvk in a per-project setting.

Signed-off-by: Colin Walters [email protected]

Some of this snuck in in the last PR.

Signed-off-by: Colin Walters <[email protected]>
Copy link

Summary of Changes

Hello @cgwalters, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This PR introduces a new --label option for bcvk libvirt run to tag VMs, and for bcvk libvirt list to filter them. It also adds a bcvk libvirt rm-all command that supports label-based bulk removal. Integration tests and cleanup scripts were updated to leverage this new labeling functionality, and relevant documentation was modified.

Highlights

  • Intent: This pull request introduces support for --label functionality within the bcvk libvirt commands. The primary goal is to enable users to assign arbitrary labels to libvirt VMs, similar to how containers are labeled, and then filter or manage these VMs based on these labels. This aims to improve organization and management of VMs, especially in per-project settings.
  • Key Changes: The changes involve several areas:
  1. Label Management for Libvirt VMs: The bcvk libvirt run command now accepts one or more --label arguments. These labels are stored as metadata within the libvirt domain XML (under bootc:label). A validation step ensures labels do not contain commas.
  2. Label-based Filtering: The bcvk libvirt list command has been enhanced with a --label option, allowing users to filter the displayed VMs by a specific label. This enables quick identification of VMs belonging to a particular project or category.
  3. Bulk Removal Command: A new command, bcvk libvirt rm-all, has been added. This command can remove multiple libvirt domains and their associated resources. Crucially, it supports filtering by --label, allowing for the mass cleanup of VMs tagged with specific labels. It also includes --force and --stop options for non-interactive or running VM removal, respectively.
  4. Integration Test Enhancements: A new shared library (crates/integration-tests/src/lib.rs) was created to centralize integration test constants. Existing integration tests for libvirt run commands were updated to include labels, and a comprehensive new test (test_libvirt_label_functionality) was added to verify the end-to-end functionality of label creation, XML storage, and label-based listing.
  5. Cleanup Utility Update: The integration test cleanup script (crates/integration-tests/src/bin/cleanup.rs) was updated to utilize the new bcvk libvirt rm-all --label command, ensuring that libvirt VMs created during tests are properly cleaned up using their assigned integration test labels.
  6. Documentation: Man pages for bcvk-libvirt-list.md and bcvk-libvirt-run.md were updated to reflect the new --label options. The bcvk-libvirt-create.md man page was removed, suggesting a consolidation of functionality.
  • Reviewer Activity: No reviewer activity was provided in the context.
Ignored Files
  • Ignored by pattern: .github/workflows/** (1)
    • .github/workflows/main.yml
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This is a great addition that improves the usability of bcvk for managing libvirt VMs in a project-based workflow. The implementation of labels, including the new rm-all command, is well thought out. I've found a few issues in the integration tests related to label consistency that could lead to test failures and resource leaks. Addressing these will make the PR solid. Great work!

This parallels what we do with `ephemeral`, where it's super
natural because containers have labels.

With libvirt we already have custom stuff in the XML, so
add support for reading/writing labels there, and filtering
by them in `bcvk libvirt list --label=foo` etc.

This makes it easier to use bcvk in a per-project setting.

Signed-off-by: Colin Walters <[email protected]>
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.

1 participant