search

fog / fog-libvirt

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

Replace server XML with Nokogiri::XML::Builder #94

Closed ekohl closed 1 year ago

ekohl commented 3 years ago

This replaces the ERB template with a real XML builder. This makes it much easier to maintain.

https://github.com/fog/fog-libvirt/issues/76 inspired this change. It would make things more flexible.

While I haven't tested this very extensively, I do believe it's ready for review. If we agree this is a good path, we can fully remove the ERB templates.

lzap commented 3 years ago

I am not a huge fan of XML builders, but I am okay with this. However I currently do not feel comfortable merging this without fixture tests. Could you add at least one fixture? Basically a rendered XML committed to git, so everytime we make a change to the builder we can compare the generated XML files. Ideally few of them for various major code paths.

There is currently lib/fog/libvirt/requests/compute/mock_files/domain.xml but I don't see a test for that, perhaps some leftover?

github-actions[bot] commented 2 years ago

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

jhoblitt commented 1 year ago

The gha logs are gone due to age. Could we reopen this PR and rebase it to trigger gha?

jhoblitt commented 1 year ago

@ekohl It looks like all that is needed for ci to pass is to let rubucop autocorrect errors.

ekohl commented 1 year ago

@jhoblitt looks like the whole RuboCop config generally just doesn't pass and needs a big update.

As for this PR itself: it would be good to have some tests on this, but I don't have time for it myself anytime soon.

jhoblitt commented 1 year ago

@ekohl Not being able to create el9 VMs via foreman is causing me major problems. What is the fastest path forward? Should I try to get this PR merged and then add machine type support to it or should I add it to the erb template?

jhoblitt commented 1 year ago

@ekohl @lzap Not being able to set pc-q35-rhel9.0.0 is killing me. Could we come up with a plan for a way forward?

ekohl commented 1 year ago

I've been on vacation for most of last month. I just rebased this with some RuboCop items addressed and tests added (that caught some bugs). I'm not sure I agree with the too high complexity issues: they were already there in the template and I think this is more readable. I could split it into multiple helpers, but I wonder if that really helps.

@jhoblitt would you have the capacity to test this PR?

jhoblitt commented 1 year ago

@ekohl Yes, I'm happy to help test. I suppose a real-world example would be to install this branch in the foreman instance for my dev env, which does have a number of libvirt compute resources configured. However, this is still an EL7 instanced (I know, I know, I'm trying to migrate) and is currently using 0.9.

tfm-rubygem-fog-libvirt-0.9.0-1.el7.noarch
jhoblitt commented 1 year ago

I think I may have seen an issue with setting boot order.

When creating a new VM, I expected to get:

  <os>
    <type arch='x86_64' machine='pc-i440fx-rhel7.6.0'>hvm</type>
    <boot dev='network'/>
    <boot dev='hd'/>
  </os>

but got 

  <os>
    <type arch='x86_64' machine='pc-i440fx-rhel7.6.0'>hvm</type>
    <boot dev='hd'/>
  </os>

I discovered there were some issues with broken plugins installed in the instance and needed to do a cleanup/reinstall. I will retest soon.

ekohl commented 1 year ago

Ah, I think I forgot the boot order. Added.

jhoblitt commented 1 year ago

Tomorrow is a major US holiday and I'll be away from the office but I'll be back on Wednesday and can do any needed testing then.

ekohl commented 1 year ago

I've noticed that RuboCop is configured to prefer double quotes but a lot of the code is in single quotes. I'm wondering if we should disable that cop instead.

ekohl commented 1 year ago

Unless other maintainers disagree I'm going to merge my own PR given @jhoblitt confirmed it works.