Add a list of interfaces #81
Conversation
* We do not want an entry in the menu for each network interface.
src/lib/y2firewall/ui_state.rb
Outdated
| # or creates a new zone. | ||
| # | ||
| # @param zone [String] zone name | ||
| def select_row(zone_name) |
There was a problem hiding this comment.
As we were discussing we should try to generalize the current. But for now we should at least fix the comments and param name as it will used not only for zones but also for interfaces.
There was a problem hiding this comment.
Just a leftover. Anyway, I will try to extract it.
src/lib/y2firewall/ui_state.rb
Outdated
| self.row_id = nil | ||
| end | ||
|
|
||
| # List of candidate nodes to go back after opening a zone view in the tree |
|
Just some minor comments. |
src/lib/y2firewall/ui_state.rb
Outdated
| # Singleton class to keep the position of the user in the UI and other similar | ||
| # information that needs to be rememberd across UI redraws to give the user a | ||
| # sense of continuity. | ||
| class UIState |
There was a problem hiding this comment.
Something like this is also used in the Partitioner. Is it easy or hard to extract the generic part?
There was a problem hiding this comment.
Yes, that's true. I took the adapted code from @teclator (thanks!) and I will try to extract the common pieces and move it to yast2.
src/lib/y2firewall/ui_state.rb
Outdated
| end | ||
|
|
||
| class << self | ||
| # Singleton instance |
There was a problem hiding this comment.
Why not use the stdlib https://devdocs.io/ruby~2.5/singleton ?
There was a problem hiding this comment.
I do not know, this code comes from yast2-storage-ng. But I do not see a good reason.
|
|
||
| module Y2Firewall | ||
| module Widgets | ||
| # A table with all {Y2Firewall::Firewalld::Zone}s. |
| DEFAULT_ZONE_NAME = "default".freeze | ||
|
|
||
| # @!attribute [r] interfaces | ||
| # @return [Array<Hash>] Interfaces |
There was a problem hiding this comment.
When a Hash is really an ad-hoc class, like it is here for the interface, I like to make a subclass of Hash just for documentation purposes, like here:
declaration: https://github.com/yast/yast-yast2/blob/e4ab10035ba30f739175009344872c4062c26db6/library/systemd/src/lib/yast2/systemd/unit_prop_map.rb#L8
reference: https://github.com/yast/yast-yast2/blob/e4ab10035ba30f739175009344872c4062c26db6/library/systemd/src/lib/yast2/systemd/socket.rb#L70
So I think we should declare Y2Firewall::Firewalld::Interface
There was a problem hiding this comment.
I agree. @teclator please, could you adapt known_interfaces to work with proper objects while I am extracting the popup and the UIState classes?
There was a problem hiding this comment.
@mvidner @imobachgs please see yast/yast-yast2#832
imobachgs
force-pushed
the
features/interfaces-list
branch
from
1c3aea0
to
847abc7
Compare
imobachgs
force-pushed
the
features/interfaces-list
branch
from
e21af0d
to
4a35f1c
Compare
imobachgs
force-pushed
the
features/interfaces-list
branch
from
4a35f1c
to
cea3a4a
Compare
imobachgs
force-pushed
the
features/interfaces-list
branch
from
cea3a4a
to
2c982d8
Compare
| # @macro seeAbstractWidget | ||
| def init | ||
| interface = Y2Firewall::UIState.instance.row_id | ||
| if interface && interfaces.map(&:name).include?(interface.to_s) |
There was a problem hiding this comment.
np: why the id is not used?
interfaces.map(&:id).include?(interface)
| def items | ||
| interfaces.map do |iface| | ||
| [ | ||
| iface.name.to_sym, |
| end | ||
|
|
||
| def selected_interface | ||
| interfaces.find { |i| i.name == value.to_s } |
| return if new_zone && new_zone.interfaces.include?(interface.name) | ||
|
|
||
| old_zone = interface.zone | ||
| old_zone.remove_interface(interface.name) if old_zone |
There was a problem hiding this comment.
Maybe a change zone method could be added to the interface object (for SP1 🙄 )
There was a problem hiding this comment.
Yes, it would be nice :)
|
LGTM |
This PR implements support to assign interfaces to firewall zones.