Conversation
Because the later is deprecated.
This commits adds a dependency toward thrnio/ip to ease matching IPv6 networks (stdlib's types only match addresses).
Without this, long lines are truncated, making Puppet 4 type matching errors unreadable.
Because there are no functional changes between 1.0.0 and 1.0.1
Because it makes rspec fail with some versions.
Of course, Puppet 4 and Puppet 5 have different messages for the same errors!
| # $ipaddress - optional | ||
| # $netmask - optional | ||
| # $macaddress - required | ||
| # $macaddress - optional |
There was a problem hiding this comment.
This was marked as "required" but accepted then ignored empty string values.
| # Only "dhcp" and "bootp" are used in /etc/sysconfig/network-scripts | ||
| # | ||
| # "static" and "none" are used in some parts of the module, | ||
| # and those values have no side effects, so we accept them too. |
There was a problem hiding this comment.
Not sure it's a good idea to keep allowing "static" and "none", as these values are not used by RedHat scripts.
But I kept them as I did not intend to introduce any regression in the PR.
| ----- | ||
|
|
||
| * Runs under Puppet 2.7 and later. | ||
| * Runs under Puppet 4.9 and later. |
There was a problem hiding this comment.
I expect this module to work on >=4.x but I am not sure what is the best method to validate this belief.
| Boolean $noaliasrouting = false, | ||
| Boolean $restart = true, | ||
| Optional[IP::Address::V4::NoSubnet] $netmask = undef, | ||
| Optional[IP::Address::V4::NoSubnet] $broadcast = undef, |
There was a problem hiding this comment.
Previous default values were misleading, and keeping them would make us accept Boolean for these parameters (doesn't sound like a good idea). This change has no impact on the way templates use the data.
| case $::operatingsystemrelease { | ||
| /^[45]/: { | ||
| case $::os['release']['major'] { | ||
| /^[45]$/: { |
There was a problem hiding this comment.
Switched to major release, as only the "major" part was used.
| :macaddress_eth1 => 'fe:fe:fe:aa:aa:aa', | ||
| :os => { | ||
| :family => 'RedHat', | ||
| :name => 'RedHat', |
There was a problem hiding this comment.
TODO: remove "name" and "release"
3 defines were using the same template, because of code duplication. As I had to update all 3 for this commit, I chose to re-use network::bridge in network::bridge::dynamic and network::bridge::static, thus removing previous code duplication.
| Optional[IP::Address::NoSubnet] $dns1 = undef, | ||
| Optional[IP::Address::NoSubnet] $dns2 = undef, | ||
| Optional[String] $domain = undef, | ||
| Boolean $restart = true, |
There was a problem hiding this comment.
network::bridge now takes takes all arguments that are used by network::bridge::dynamic and network::bridge::static in order to prevent code duplication.
| $effective_hostname = $hostname ? { | ||
| String => $hostname, | ||
| default => $::networking['fqdn'], | ||
| } |
There was a problem hiding this comment.
This is ugly, but was the only way to preserve the current behavior:
- When called with a
$hostname, it is used both byhostnamectland the template - When called without a
$hostname, the template gets a value through facts, but thehostnamectlexecisn't evaluated
I know the result is the same, but to simplify the code, we could use $::networking['fqdn'] as default value for $hostname, and let the unless attribute of the exec resource do its job.
What do you think?
|
@razorsedge How can I improve this PR ? |
|
+1'000: I'd like to see this merged so bad.... |
|
Same here, we are on Puppet 5 and https://github.com/voxpupuli/puppet-network has many issues for us, so we'd like to migrate to this module here, but this is one of the things preventing us... |
3 participants
No description provided.