Skip to content
This repository has been archived by the owner on May 4, 2023. It is now read-only.

Permit disabling serial console if there is none #5

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

matsimon
Copy link

@matsimon matsimon commented Dec 11, 2018

SUMMARY

The serial console should be enabled by default, hence the introduction a default value enabling if not explicitely disabled. This does NOT change the default behaviour which is intended.

Verified with: HPE ProLiant MicroServer Gen10 which has neither any iLO-based BMC with virtual serial port, nor any physical serial port by default.

ISSUE TYPE
  • Feature Pull Request
  • Bugfix Pull Request

(actually a bit both)

ANSIBLE VERSION
ansible 2.7.4
  config file = /etc/ansible/ansible.cfg
  configured module search path = (removed)
  ansible python module location = /usr/lib/python2.7/dist-packages/ansible
  executable location = /usr/bin/ansible
  python version = 2.7.13 (default, Sep 26 2018, 18:42:22) [GCC 6.3.0 20170516]
ADDITIONAL INFORMATION

The serial console should be enabled by default, hence the
introduction a default value enabling if not explicitely disabled.

Verified with: HPE ProLiant MicroServer Gen10 which has neither
               an iLO-based BMC with virtual serial port, nor
               any physical serial port by default.
@matsimon matsimon requested a review from strixBE December 11, 2018 13:47
Copy link
Contributor

@keachi keachi left a comment

Choose a reason for hiding this comment

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

Mostly good, just one little thing. A little bit of nitpicking.

@@ -16,9 +16,12 @@ GRUB_TIMEOUT={{ grub_timeout }}
GRUB_DISTRIBUTOR=$(lsb_release -i -s 2>/dev/null || echo {{ ansible_distribution }})
GRUB_CMDLINE_LINUX_DEFAULT="{{ grub_console }}"
GRUB_CMDLINE_LINUX="{{ grub_cmdline_linux | join(' ') }}"
{% if grub_serial | d(False) %}
{% if grub_serial_enabled == True %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Just write {% if grub_serial_enabled %}, as this is shorter

Copy link
Author

Choose a reason for hiding this comment

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

Right, fixed with aa5de19

GRUB_SERIAL_COMMAND="serial --speed={{ grub_serial.speed }} --unit={{ grub_serial.unit }} --word={{ grub_serial.word }} --parity={{ grub_serial.parity }} --stop={{ grub_serial.stop }}"
{% endif %}

# disable graphical terminal (grub-pc only)
{% if grub_serial_enabled == True %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Just write {% if grub_serial_enabled %}, as this is shorter

Copy link

Choose a reason for hiding this comment

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

Why two consecutive equal if-statements?

Copy link
Author

Choose a reason for hiding this comment

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

True, fixed with aa5de19

GRUB_TERMINAL=console
{% endif %}
Copy link

Choose a reason for hiding this comment

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

This endif has no corresponding if-block.

Copy link
Author

@matsimon matsimon Dec 18, 2018

Choose a reason for hiding this comment

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

Now it should be since aa5de19

@@ -5,6 +5,8 @@ grub_consoles:
- tty0
- 'ttyS0,{{ grub_serial.speed }}'

grub_serial_enabled: True

# grub serial command settings
grub_serial:
Copy link

Choose a reason for hiding this comment

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

Not sure about this one. In my mind it makes more sense to add grub_serial_enabled to the grub_serial-block:

Suggested change
grub_serial:
grub_serial:
enabled: True

Copy link
Author

Choose a reason for hiding this comment

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

I have actually tried this, the issue was that if you have a system with no serial console you'd actually have to override "enabled" + ALL the other elements needlessly (speed, unit, word, parity and stop) since if you only define i.e.

grub_serial:
  enabled: False

It will fail because you end up with the remaining element being undefined which trips Ansible.

* {% if grub_serial_enabled == True %} -> {% if grub_serial_enabled %}
* Remove unneeded double if statement
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants