Partial VLAN support refactor #98
Conversation
With this refactor I implement the idea that the provider should just check for the right syntax (i.e. all the needed options are passed to it) instead of trying to reimplement what Debian *vlan-interfaces* already does.
I removed the old eth0.1 test because it does not really test anything new, because the vlan-raw-device option should be present only if specified and not always. Maybe I should add another eth0.1 test to see if the vlan regexp raise an error when a mispelled vlan is found
|
Sorry, I'm trying to fix the Travis build, don't know what's happening there :( |
|
I have no idea about why Travis is complaining about augeas in network::bond, I didn't touch anything of it. Any idea? |
|
I retried the tests and they still fail |
|
Running the tests manually, its failing on master right now |
|
@adrienthebo thanks for throwing me under the bus. :) I need to think about this some more (read, I need to sit down and play with it) but I think it would be a real shame if vlan interface setup required different syntax between EL and Debian. |
|
@jhoblitt actually there are still no differences between a vlan interfaces under Debian and RHEL. The only thing I changed is the interfaces names accepted as being VLANs (vlanNN + ethX.NN) and that we don't try to reimplement (poorly) what vlan-interfaces already does, like for example trying to figure out what the raw device is, because it's Debian job to do it and we must support that a user could specify a different raw device from the one guessed from the name (which is something Debian's vlan-interfaces supports, just take a look at man 5 vlan-interfaces). To resume, my approach is: don't reimplement, just check syntax and throw errors if something is wrong in the parameters we received. Still no clue why tests are failing, though. :( |
|
Hello: What's the status of this PR? |
|
@jyaworski I don't know and, to be honest, I don't care. We are still using our fork that meets our needs, feel free to use it as well, if I have more spare time I'll try to resync with the current state of puppet-network module and see if it still fits. I had no response to my lengthy post in almost a year now, so I guess the maintainer is not interested in this PR anyway (sorry for being this passive-aggressive but I can't help) |
|
@vide no problem. I became a collaborator recently and I'm checking old PRs. Feel free to update; you'll get a response this time. |
| if provider.options["vlan-raw-device"] | ||
| stanza << "vlan-raw-device #{provider.options["vlan-raw-device"]}" | ||
| else | ||
| vlan_range_regex = %r[\d{1,3}|40[0-9][0-5]] |
There was a problem hiding this comment.
@vide can you explain this regex? If I'm reading it right it looks for three consecutive digits or four in a weird range between 4000-4095, excepting when the last digit is 6-9. Vlan 4006 should be valid, for instance. Then in your test you use vlan 4500 as an ID. What am I missing?
There was a problem hiding this comment.
It was the regexp used before #116 was fixed, I didn't write it :)
There was a problem hiding this comment.
I assume your fork has that updated regex then, but the rest remains the same? If we take it in now, it would be nice to know it matches your fork, since you've given that a field test.
There was a problem hiding this comment.
@rnelson0 well my fork is quite old and it needs a rebase against current master to be useful at all. If thwere's real interesting in accepting this feature and merging it, I could work on it
There was a problem hiding this comment.
@vide Yes, we are definitely interested in moving this module forward and would appreciate any and all help!
|
Ping @vide. |
|
Hi @vide, could you please have a look and rebase? |
|
@bastelfreak @jyaworski sorry for the delay, I was very busy :( I'll do my best to have the PR rebased this week! |
|
Closing since the other pr was merged |
With this refactor I implement the idea that the provider should just check for the right syntax (i.e. all the needed options are passed to it) instead of trying to reimplemen what Debian vlan-interfaces already does.
I've already talked a little about this with @adrienthebo on IRC and he seemed to agree with me, but he told me too that the real active mantainer at the moment is @jhoblitt . So, @jhoblitt , are you OK with this change?
Thanks!