Skip to content

Network type and provider #47

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 15 commits into
base: master
Choose a base branch
from

Conversation

jaggededgedjustice
Copy link

This is an alternative to #25 using the libvirt api instead of shelling out to virsh, and maintains the current resource definition interface.
This only a demonstartion at this point, to be useable it would require converting the settings to properties. It also lacks working tests.

@igalic
Copy link
Contributor

igalic commented Mar 23, 2015

i think i'd leave the defined type in for now, and replace its guts with a call to the type.
that way we may be able to reuse the tests, maybe…

@jaggededgedjustice
Copy link
Author

I think keeping the defined type will only increase the complexity with the added layers. The current tests in spec/defines/network_spec.rb are checking that defining a resource produses an exec resource with the correct command, so they couldn't be used as is.

@jaggededgedjustice
Copy link
Author

I think the curent state should sufice to show the design for the provider. Although this will only update the stored config so the network would need to restarted to affect the running config.
The main issue now is the lack of tests to ensure there are no regressions with this. Is there likly to be any movement on #33 in the near future?

@igalic
Copy link
Contributor

igalic commented Mar 30, 2015

@jaggededgedjustice i haven't had any time :(mostly due to my involvement in @puppet-community):
but i hope @thias comes around to reviewing / merging this one here

@beddari
Copy link

beddari commented May 1, 2015

This is great! It looks like the same code could be used to manage non-libvirt interfaces too, like https://github.com/raphink/puppet-netcf

@raphink
Copy link

raphink commented May 1, 2015

@beddari most likely, since libvirt makes use of netcf internally.

end

def create
# @property_hash = @resource
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason why this code is commented out?
can we do better here? Or should we just remove #create altogether, seeing as this is done in #flush?

Copy link
Author

Choose a reason for hiding this comment

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

The commented out line is leftover from some experimentation, I'll remove it. The create function must exist, even if it's empty, otherwise puppet will throw an error.

@igalic
Copy link
Contributor

igalic commented May 6, 2015

@jaggededgedjustice do you have any opinions on @beddari's and @raphink's comments?
otherwise, a good (re)start for this Pull Request would be a rebase.

@jaggededgedjustice
Copy link
Author

@beddari, It might be possible, I've never encountered netcf before. Although the config schema for netcf and libvirt looks much the same there are some differences, such as the open vswitch ports that libvirt has which netcf does not. It may be possible to share the code for parsing the current state of a resource between this project and pupet-netcf. But maybe not, libvirt has its own config parser/generator which is used internally, so although the two schemas look the same they might be different enough that sharing code is pointless. Though it would be worth looking into in the future.

@igalic
Copy link
Contributor

igalic commented May 21, 2015

@jaggededgedjustice do you wanna pick this up again and rebase it?
if you don't, i'll do it :P

@jaggededgedjustice
Copy link
Author

I'm hoping to be able to get the rebase done this weekend, RL has been conspiring against me recently.

@jaggededgedjustice
Copy link
Author

k, rebase done

@jaggededgedjustice jaggededgedjustice changed the title POC Network type Network type May 23, 2015
@jaggededgedjustice jaggededgedjustice changed the title Network type Network type and provider May 23, 2015
@igalic
Copy link
Contributor

igalic commented May 23, 2015

superb. I'll try rewrite the unit tests next week.

@igalic
Copy link
Contributor

igalic commented May 26, 2015

@thias ping / merge :D

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.

5 participants