Skip to content

Allow Libvirt provisioner custom settings #124

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

br0ziliy
Copy link
Collaborator

This is WIP for RFE #121. Not (yet) tested, so please don't yet merge, I have couple of things I don't like in this diff, and want some 2nd opinion:

  • argv processing. Can't we finally rewrite command line arguments block to use getopt?
  • assignments on line 1519 and further - can this be simplified somehow, or it's fine as it is?

@purpleidea
Copy link
Owner

@br0ziliy Sweet... Okay some replies:

  1. Re: argv processing, we should definitely "libify" this and use getopt or whatever ruby thing is best if it will work. So that should be a separate issue (and even a pre-req to this one) and I'll happily accept that patch. Your patch as is, I like though!

Small bug: in the parser add on, I think it won't work correctly for the connect_via_ssh key, since that's a bool, not a string, and your parse always assumes string values.

  1. Re: line 1519: I think it looks fine, but maybe there is a ruby idiom I don't know about. Better to ask someone else, otherwise I'm fine to merge that part.

  2. One small nitpick: can you move the :libvirt key one up so it's above the comment field, and so comment is always last in the omv.yaml file? I think it will be a bit cleaner this way. Remember that this patch should also include empty :libvirt keys to all the existing example files please :)

Obviously doc patches are appreciated, but I wouldn't block the merge without them.
It looks amazing! 10/10, would review again. Thanks!

@purpleidea
Copy link
Owner

@br0ziliy ping, want to update/rebase this lovely patch so we can merge it?

Thanks!

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.

2 participants