search

fog / fog-libvirt

libvirt provider for fog
MIT License
16 stars 42 forks source link

Add flags option to server.destroy #102

Closed byonoy-dne closed 2 years ago

byonoy-dne commented 2 years ago

A call to server.destroy ultimately ends in a call to virDomainUndefineFlags. Right now there is no way to set the flags argument with which virDomainUndefineFlags gets called. This patch provides such an option to the caller.

This is needed to address vagrant-libvirt#1027. Specifically, vagrant-libvirt needs to be able to set the VIR_DOMAIN_UNDEFINE_NVRAM flag when calling destroy on a domain configured with NVRAM.

There is one gotcha to this. If libvirt-ruby is built without HAVE_VIRDOMAINUNDEFINEFLAGS the call does not end in virDomainUndefineFlags but in virDomainUndefine. In that case a non-zero flags argument will result in an error. I have found no documentation or reference to that build flag. All I know for sure is that the package in the Fedora 34 repo is built with it enabled, and the user who created aformentioned issue in vagrant-libvirt also has such a build. But given that virDomainUndefineFlags was implemented seven years ago, it is probably ok?

Beware, this is my first contact with ruby. I hope I didn't do anything completely stupid...

github-actions[bot] commented 2 years ago

This pr has been marked inactive and will be closed if no further activity occurs.

lzap commented 2 years ago

For the record the similar patch is here: https://github.com/fog/fog-libvirt/pull/107

The difference is that this one uses hash to pass options, which seems more robust to me.

lzap commented 2 years ago

Damn:

  For more information: https://docs.rubocop.org/rubocop/versioning.html
  reviewdog: found at least one result in diff
  reviewdog: This GitHub token doesn't have write permission of Review API [1], 
  so reviewdog will report results via logging command [2] and create annotations similar to
  github-pr-check reporter as a fallback.

I guess it was me who broke this. https://github.com/fog/fog-libvirt/pull/106

lzap commented 2 years ago

Oh it was just a warning, please turn off this cop:

  Error: [rubocop] reported by reviewdog 🐶
  Class has too many lines. [376/372]

  Raw Output:
  C:  8:  7: Metrics/ClassLength: Class has too many lines. [376/372]

I hate mertics and style alltogether, maybe to disable them all? @ekohl

byonoy-dne commented 2 years ago

Oh it was just a warning, please turn off this cop:

Is this addressed to me or @ekohl? Should I make this part of this PR?

ekohl commented 2 years ago

I hate mertics and style alltogether, maybe to disable them all? @ekohl

It kind of depends. Generally I like to think Rubocop has mostly sane defaults, but not for everything. Perhaps we should disable Metrics/ClassLength in that file for now?

lzap commented 2 years ago

@byonoy-dne just make Rubocop happy, you may disable that pesky rule (not all tho). Thanks!