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

[WIP] Add domain_info #146

Closed
wants to merge 3 commits into from
Closed

Conversation

mlow
Copy link
Contributor

@mlow mlow commented Oct 20, 2022

SUMMARY

Adds a domain_info module, specializing in retrieving information about a specific domain.

See #144 for the rational behind this addition.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

domain_info

ADDITIONAL INFORMATION

I'm mainly putting this out there looking for feedback at the moment!

mlow added 3 commits October 20, 2022 16:45
Rather than nesting requested data in 'info' key, return it at the
same level as the rest.

Also bypass argument validation and use the return from _load_params()
to check if params have been provided. Removes the need to supply the
empty dict syntax in the YAML.
@dseeley
Copy link
Contributor

dseeley commented Oct 21, 2022

Is it worth considering following the existing names as used for this in the inventory module https://github.com/ansible-collections/community.libvirt/blob/main/plugins/inventory/libvirt.py (xml_desc, guest_info, interface_addresses)?

I also wonder whether domain_info is the best name for this module, as domain.info() is already an API call, whose results aren't actually included in the information returned by this module, and might be confusing? Perhaps include the results of domain.info() as info, (as in inventory/libvirt.py), and rename the module to domain_extended_info (or something)?

@mlow
Copy link
Contributor Author

mlow commented Oct 22, 2022

Is it worth considering following the existing names as used for this in the inventory module https://github.com/ansible-collections/community.libvirt/blob/main/plugins/inventory/libvirt.py (xml_desc, guest_info, interface_addresses)?

I also wonder whether domain_info is the best name for this module, as domain.info() is already an API call, whose results aren't actually included in the information returned by this module, and might be confusing? Perhaps include the results of domain.info() as info, (as in inventory/libvirt.py), and rename the module to domain_extended_info (or something)?

I agree it might cause some confusion.. I stumbled on this during writing; the reason I had not (yet) implemented an info field is that domain.info() is only a subset of what dominfo provides, and my general goal has been to resemble the CLI (which I didn't already with xml). That said, I'm happy to rename the fields to match the names in the inventory plugin.

I think we might be able to stick with domain_info if we return the values of domain.info() by default (including state, which will no longer be an opt-in field). I wouldn't be against renaming it to dominfo to match the CLI.

What do you think?

Copy link
Contributor

@Andersson007 Andersson007 left a comment

Choose a reason for hiding this comment

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

@mlow hello, thanks for working on the new module! Nice to see people adding new content to the collection!

Could you please:

  • Add [WIP] to the PR's description
  • Add version_added: tag (use the next minor release number)
  • Make the docs satisfying the style guide (including the paragraph about using macros)
  • Be sure the module supports check mode (also please add notes: section with - Supports C(check_mode).
  • Add integration tests. See the content of tests/integration/targets and the integration test quick start guide.
  • When it's ready for review, remove [WIP] from the description

@mlow mlow changed the title Add domain_info [WIP] Add domain_info Oct 24, 2022
@csmart
Copy link
Collaborator

csmart commented Feb 28, 2023

Hi @mlow, thanks again for this contribution! I just want to follow up on the items mentioned above, do you think you will have time to update this? Thanks!

@mlow
Copy link
Contributor Author

mlow commented Mar 1, 2023

Hi, sorry to have left this hanging. I do expect I'll have some time to work on this coming up.

My current concern with integration testing is that I'm assuming we can't create KVM domains in the environment (am I wrong?); will it be possible to create LXC domains?

@mlow mlow marked this pull request as draft March 8, 2023 17:37
@mlow
Copy link
Contributor Author

mlow commented Oct 9, 2023

Closing this for now. I have a body of work (related to this) that feels like it would fit well into community.libvirt, but currently don't have the time to drive it towards being ready for submission.

@mlow mlow closed this Oct 9, 2023
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.

4 participants