Grub_pass & ks_console

TimoGoebel · July 31, 2018, 6:15pm

Hi devs,

as part of the template rendering refactoring, I want to remove UnattendedHelper.

The helper defines two macros: grub_pass and ks_console. Both macros rely on the instance variables @port, @baud and @grub being set.

github.com

theforeman/foreman/blob/f715a7c98457c6f43a1f803a75ba4540372d8544/app/helpers/unattended_helper.rb#L2-L12


def ks_console
  (@port && @baud) ? "console=ttyS#{@port},#{@baud}" : ""
end


def grub_pass
  if @grub
    @host.grub_pass.start_with?('$1$') ? "--md5pass=#{@host.grub_pass}" : "--iscrypted --password=#{@host.grub_pass}"
  else
    ""
  end
end

As far as I’m concerned, these instance variables are defined nowhere and the code is pretty much dead. It actually has been dead for at least >= Foreman 0.4.

Are they still needed?

Timo

tbrisker · August 1, 2018, 10:29am

i-see-dead-code

But I would be happy if @lzap confirms it truly is since I think he’s most familiar with this.

Marek_Hulan · August 1, 2018, 12:54pm

Both would need to be removed from kickstart template. There’s no guarantee people don’t use it in their custom templates where they set some value to these variables. I think we should either ask users first, this thread is tagged as Development. Or at least provide a deprecation warning or drawer notification if they are used before we remove them.

cc @ohadlevy since IIRC he was aware of someone using ks_console

tbrisker · August 1, 2018, 1:00pm

while the macro is in the template, the code seems to indicate that these two both return empty strings since @grub, @port and @baud do not seem to be set anywhere. If we want to be on the safe side, we can add a deprecation warning to these and remove in 1.21.

TimoGoebel · August 1, 2018, 1:12pm

I’d also be happy to make these macros work again, but I’d need some help how these macros are supposed to be used.

ekohl · August 1, 2018, 1:12pm

The git log suggests they used to be set via host parameters but I’m not sure that still worked when they were refactored into helpers.

TimoGoebel · August 1, 2018, 1:29pm

That’s interesting, I have not found this while digging in the code. The only use case I could imagine was having a template like this

<% @grub = true %>
<%= grub_pass %>

But that does not read very good.

ohadlevy · August 7, 2018, 2:24pm

I can’t remember anyone using it now atm, maybe we can somehow let users know about deprecation after an upgrade using the notification drawer? (or even scan if they have deprecated functions and then let them know…?)

TimoGoebel · August 20, 2018, 3:27pm

@ohadlevy: Do you still remember how this was supposed to be used in the initial implementation?