Fix wrong interface name with auto_vlan=yes #39
Conversation
warning: assigned but unused variable
ported from SLE11, 5db8f5b
|
Changes Unknown when pulling 85610b1 on vlan-12 into ** on SLE-12-SP2**. |
src/modules/FcoeClient.rb
Outdated
| ), | ||
| "nfsroot" | ||
| ) | ||
| SCR.Write(ifcfg_path + "STARTMODE", "nfsroot") |
There was a problem hiding this comment.
this can be picked out of if-else - shared between both branches
src/modules/FcoeClient.rb
Outdated
| ) | ||
| SCR.Write(ifcfg_path + "BOOTPROTO", "static") | ||
| SCR.Write(ifcfg_path + "STARTMODE", "nfsroot") | ||
| SCR.Write(ifcfg_path + "NAME", Ops.get_string(card, "device", "")) |
There was a problem hiding this comment.
why not to get even rid of last remaining Ops.get_string(...)?
| dev_name, | ||
| Ops.get_string(card, "vlan_interface", "") | ||
| ) | ||
| cmd_ifup = Builtins.sformat("ifup %1.%2", dev_name, vlan_interface) |
There was a problem hiding this comment.
we have SetIfaceUp in yast2-network ;-)
There was a problem hiding this comment.
Having learned about it, well, it is such a lone function (nobody uses it!) that I don't feel comfortable using it.
BTW it does not document its return value: http://www.rubydoc.info/github/yast/yast-network/Yast%2FNetworkRoutinesInclude%3ASetIfaceUp
There was a problem hiding this comment.
well it used to be used. Currently it is replaced by something more friendly to wicked.
src/include/fcoe-client/complex.rb
Outdated
| if card["auto_vlan"] == "yes" || vlan_interface == "0" | ||
| command = Builtins.sformat("fipvlan -c -s -f '-fcoe' %1", dev_name) | ||
| else | ||
| command = Builtins.sformat("fipvlan -c -s %1", dev_name) |
There was a problem hiding this comment.
You did a nice cleanup, so you can go even further: "fipvlan -c -s #{ dev_name}"
| def HandleConfigurationDialog(id, event) | ||
| event = deep_copy(event) | ||
| action = Ops.get(event, "ID") | ||
| def HandleConfigurationDialog(_id, _event) |
There was a problem hiding this comment.
what is the reason why this cannot be removed completely?
There was a problem hiding this comment.
We could, but I just have to stop the refactoring somewhere.
There was a problem hiding this comment.
you're touching it anyway and dropping unused method makes more sense to me over adding '_' to satisfy a convention ;-)
There was a problem hiding this comment.
An empty method is not an unused method. You have missed my point.
| Ops.get_string(card, "fcoe_vlan", "") != @NOT_CONFIGURED | ||
| fcoe_vlan = card.fetch("fcoe_vlan", "") | ||
| if fcoe_vlan != @NOT_AVAILABLE && | ||
| fcoe_vlan != @NOT_CONFIGURED |
There was a problem hiding this comment.
alternative which I sometimes prefer: ![@NOT_AVAILABLE, @NOT_CONFIGURED].include? fcoe_vlan
|
Changes Unknown when pulling 385781c on vlan-12 into ** on SLE-12-SP2**. |
|
Changes Unknown when pulling e12fa5d on vlan-12 into ** on SLE-12-SP2**. |
|
Changes Unknown when pulling c56f146 on vlan-12 into ** on SLE-12-SP2**. |
Porting #38 to SLE-12
The actual patch has a couple of lines, the rest of the PR makes its surroundings a bit more readable.