Skip to content
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

Fix ip_route for appliction_console #257

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Dec 9, 2024

On typical systems, ip_route calls ip route show default On the mac, ip_route calls ip route get while

This fixes ip_route from failing and fixes an appliance_console bug

before

# LinuxAdmin::NetworkInterface.list
route: writing to routing socket: not in table (LinuxAdmin::NetworkInterfaceError)
 error was: route: writing to routing socket: not in table
 error was: route: writing to routing socket: not in table

	from /Users/kbrock/src/gems/linux_admin/lib/linux_admin/network_interface.rb:26:in `list'
	from (irb):2:in `<main>'
	from <internal:kernel>:187:in `loop'
	from railties (7.0.8.6) lib/rails/commands/console/console_command.rb:74:in `start'
	from railties (7.0.8.6) lib/rails/commands/console/console_command.rb:19:in `start'

after

# LinuxAdmin::NetworkInterface.list.first
=>
#<LinuxAdmin::NetworkInterfaceDarwin:0x0000000163fd2d98
 @interface="lo0",
 @link_type="loopback",
 @network_conf={:address=>"127.0.0.1", :prefix=>8, :mac=>"00:00:00:00:00:00", :gateway4=>"192.168.1.1", :gateway6=>"fe80::%utun0"}>

On typical systems, ip_route calls `ip route show default`
On the mac, ip_route calls `ip route get while`

This fixes `ip_route` from failing and fixes an appliance_console bug
@kbrock kbrock added the bug label Dec 9, 2024
@miq-bot
Copy link
Member

miq-bot commented Dec 9, 2024

Some comments on commit kbrock@e9e80e9

spec/network_interface_spec.rb

  • ⚠️ - 45 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

@miq-bot
Copy link
Member

miq-bot commented Dec 9, 2024

Checked commit kbrock@e9e80e9 with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
7 files checked, 4 offenses detected

lib/linux_admin/distro.rb

lib/linux_admin/homebrew.rb

end
end
end

Copy link
Member

Choose a reason for hiding this comment

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

Look like you have a bonus line at the end here.

Comment on lines 16 to +19
if [Distros.rhel, Distros.fedora].include?(Distros.local)
NetworkInterfaceRH
elsif Distros.local == Distros.darwin
NetworkInterfaceDarwin
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this is cleaner as:

Suggested change
if [Distros.rhel, Distros.fedora].include?(Distros.local)
NetworkInterfaceRH
elsif Distros.local == Distros.darwin
NetworkInterfaceDarwin
case Distros.local
when Distros.rhel, Distros.fedora
NetworkInterfaceRH
when Distros.darwin
NetworkInterfaceDarwin

@Fryguy
Copy link
Member

Fryguy commented Dec 9, 2024

Overall looks good - can you add specs for ip_route method?

@Fryguy Fryguy self-assigned this Dec 9, 2024
@Fryguy
Copy link
Member

Fryguy commented Dec 9, 2024

My only other thought is, if we're going all the way to Darwin, should we try to avoid having to install iproute2mac, and use the built-in tools instead?

@agrare
Copy link
Member

agrare commented Dec 9, 2024

I do think at a high level we need to answer "do we want linux_admin to work on mac?" (emphasis on linux here)

We can get some of the networking commands to work but there are some other command sets which will not (systemd vs old init.d service, rpm packages, etc...) and we could need something like supports here.

@agrare
Copy link
Member

agrare commented Dec 9, 2024

I think the easy answer is "if it isn't difficult to get it to work we might as well" but how far do we want to go in order to get it to work?
Also the specs should 100% be able to be run on a mac (here and appliance_console).

@Fryguy
Copy link
Member

Fryguy commented Dec 9, 2024

I'm ok with bringing in things bit by bit to support Mac. Heck, I'd be ok with supporting WSL too at some point :)

Agreed on something like supports here would be good. At a minimum we just raise NotImplementedError for certain things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants