Skip to content

Conversation

carbonin
Copy link
Member

Added a method for setting the host name via hostnamectl to the Hosts class.

Copy link
Member

Choose a reason for hiding this comment

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

Is all of LinuxAdmin systemctl based? If not, can we make this so that it does it properly depending on the system? @bdunne Thoughts on organizing this?

Copy link
Member Author

Choose a reason for hiding this comment

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

So traditionally /etc/hostname and the hostname command were used to deal with this.

With systemd systems we got the new systemd-hostnamed service which seems to be adding a layer of complexity to this.

For example:
I would have expected running hostnamectl set-hostname new-host to have the same effect as echo "new-host" > /etc/hostname && hostname -F /etc/hostname, but after running the latter commands hostnamectl status still shows the old name and doesn't change until I run systemctl restart systemd-hostnamed

So I'm thinking like this:

if cmd?("hostnamectl")
  run!(cmd('hostnamectl'), :params => ['set-hostname', name])
else
  File.write("/etc/hostname", name)
  run!(cmd('hostname'), :params => ["-F", "/etc/hostname"])
end

What do you think @Fryguy ?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that sounds good to me...We might want a higher level check for systemd instead of the individual commands, but this should work.

Also the params on the second one can be simplified to :params => {:F => "/etc/hostname"} Note, if there's a full-word version of -F, let's use that...it documents the code better and doesn't lead to googling around for what that parameter means.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it's --file so I'll use :params => {:file => "/etc/hostname"}

But on a side note:

irb(main):004:0> AwesomeSpawn::CommandLineBuilder.new.build("hostname", {:F => "/etc/hostname"})
=> "hostname --F /etc/hostname"

Am I missing something?

Copy link
Member

Choose a reason for hiding this comment

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

oh noes. That is a bug. single letter

@carbonin carbonin changed the title Added method to set hostname to Hosts class Added method to get and set hostname to Hosts class Sep 25, 2015
Copy link
Member

Choose a reason for hiding this comment

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

We already require activesupport/core_ext so you could simplify this if there is nothing on output when unsuccessful.

run(cmd("hostname")).output.presence

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure about what will be on output. It depends really on what Open3.capture3 returns here (https://github.com/ManageIQ/awesome_spawn/blob/master/lib/awesome_spawn.rb#L119) and what hostname returns in whatever failure case happened.

@bdunne
Copy link
Member

bdunne commented Sep 28, 2015

👍 looks good, just the one comment.

@kbrock
Copy link
Member

kbrock commented Sep 28, 2015

This is nice.

I think being explicit around success? is nice. probably overkill but... kinda like it.

👍 LGTM

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we want to add the logic to add the machine's IP address to the /etc/hosts file (https://github.com/ManageIQ/manageiq-appliance/blob/master/LINK/usr/bin/miqnet.sh#L167) here or have the caller do it in a separate call?
@Fryguy @bdunne

Copy link
Member

Choose a reason for hiding this comment

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

If you are setting ip address, would you ensure that if you set a different ip address, it clears out the old ip address from /etc/hosts
If you are changing the hostname, do we also clear that out.

man, the use cases around this are complicated.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would say that the safest way to go would be to do as little as possible with each call. I don't really want to assume what people will want to do with these tools. I think it will be more clear from the calling code to do something like this https://github.com/ManageIQ/manageiq/pull/4558/files#diff-16bd2bb3c0abfe4c0381896614e7b987R238

@miq-bot
Copy link
Member

miq-bot commented Sep 29, 2015

Checked commits carbonin/linux_admin@5f550ba~...44398ae with ruby 1.9.3, rubocop 0.34.2, and haml-lint 0.13.0
2 files checked, 0 offenses detected
Everything looks good. 🍪

gtanzillo added a commit that referenced this pull request Sep 29, 2015
Added method to get and set hostname to Hosts class
@gtanzillo gtanzillo merged commit 49a6c47 into ManageIQ:master Sep 29, 2015
@carbonin carbonin deleted the add_hostnamectl_method_to_hosts branch October 2, 2015 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants