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

Add 'state' parameter for alternatives #4557

Merged
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
minor_changes:
- alternatives - add ``state`` parameter, which provides control over whether the alternative should be set as the active selection for its alternatives group (https://github.com/ansible-collections/community.general/issues/4543, https://github.com/ansible-collections/community.general/pull/4557).
75 changes: 67 additions & 8 deletions plugins/modules/system/alternatives.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,16 @@
- The priority of the alternative.
type: int
default: 50
state:
description:
- C(present) - install the alternative (if not already installed), but do
not set it as the currently selected alternative for the group.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe to clarify:

Suggested change
not set it as the currently selected alternative for the group.
not set it as the currently selected alternative for the group. If it already
is the selected alternative for the group, it will not change that.

- C(selected) - install the alternative (if not already installed), and
set it as the currently selected alternative for the group.
choices: [ present, selected ]
default: selected
type: str
version_added: 4.8.0
requirements: [ update-alternatives ]
'''

Expand All @@ -61,6 +71,13 @@
name: java
path: /usr/lib/jvm/java-7-openjdk-i386/jre/bin/java
priority: -10

- name: Install Python 3.5 but do not select it
community.general.alternatives:
name: python
path: /usr/bin/python3.5
link: /usr/bin/python
state: present
'''

import os
Expand All @@ -70,6 +87,15 @@
from ansible.module_utils.basic import AnsibleModule


class AlternativeState:
Copy link
Collaborator

Choose a reason for hiding this comment

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

A bit of a nit-picking but I think both terms in the name should be plural:

  • the name of the module is "alternatives" so we should be consistent
  • within this class we have more than one state, so it should be "states". Line 109 choices=AlternativeState.to_list() sounds quite odd - to me at least.

Also I think we might want to consider prefixing that class name with a _, so that it doesn't become a public symbol. By being public, any change we make later on will require a long deprecation period.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There are many arguments for and against using a plural vs. a singular name for an enumeration. Java and C# seem to prefer singular names (https://stackoverflow.com/a/15756009). I guess in the end it's a matter of taste.

Also I think we might want to consider prefixing that class name with a _, so that it doesn't become a public symbol. By being public, any change we make later on will require a long deprecation period.

I wouldn't consider modules (as opposed to module_utils etc.) a public API. The guidelines only talk about public/private module and plugin utils. Maybe we should discuss this in community-topics...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fair enough. I reckon this is not a show stopper.

PRESENT = "present"
SELECTED = "selected"

@classmethod
def to_list(cls):
return [cls.PRESENT, cls.SELECTED]


def main():

module = AnsibleModule(
Expand All @@ -78,6 +104,11 @@ def main():
path=dict(type='path', required=True),
link=dict(type='path'),
priority=dict(type='int', default=50),
state=dict(
type='str',
choices=AlternativeState.to_list(),
default=AlternativeState.SELECTED,
),
),
supports_check_mode=True,
)
Expand All @@ -87,6 +118,7 @@ def main():
path = params['path']
link = params['link']
priority = params['priority']
state = params['state']

UPDATE_ALTERNATIVES = module.get_bin_path('update-alternatives', True)

Expand Down Expand Up @@ -126,9 +158,20 @@ def main():
link = line.split()[1]
break

changed = False
if current_path != path:

# Check mode: expect a change if this alternative is not already
# installed, or if it is to be set as the current selection.
if module.check_mode:
module.exit_json(changed=True, current_path=current_path)
module.exit_json(
changed=(
path not in all_alternatives or
state == AlternativeState.SELECTED
),
current_path=current_path,
)

try:
# install the requested path if necessary
if path not in all_alternatives:
Expand All @@ -141,18 +184,34 @@ def main():
[UPDATE_ALTERNATIVES, '--install', link, name, path, str(priority)],
check_rc=True
)
changed = True

# select the requested path
# set the current selection to this path (if requested)
if state == AlternativeState.SELECTED:
module.run_command(
[UPDATE_ALTERNATIVES, '--set', name, path],
check_rc=True
)
changed = True

except subprocess.CalledProcessError as cpe:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this not covered by check_rc=True?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, actually subprocess.CalledProcessError should never leave module.run_command(). The module has been catching it before, I guess both the old catch and the new one can be removed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That can also be done later.

module.fail_json(msg=str(dir(cpe)))
elif current_path == path and state == AlternativeState.PRESENT:
# Case where alternative is currently selected, but state is set
# to 'present'. In this case, we set to auto mode.
if module.check_mode:
module.exit_json(changed=True, current_path=current_path)

changed = True
try:
module.run_command(
[UPDATE_ALTERNATIVES, '--set', name, path],
check_rc=True
[UPDATE_ALTERNATIVES, '--auto', name],
check_rc=True,
)

module.exit_json(changed=True)
except subprocess.CalledProcessError as cpe:
module.fail_json(msg=str(dir(cpe)))
else:
module.exit_json(changed=False)

module.exit_json(changed=changed)


if __name__ == '__main__':
Expand Down
7 changes: 7 additions & 0 deletions tests/integration/targets/alternatives/tasks/main.yml
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,12 @@
# Test that path is checked: alternatives must fail when path is nonexistent
- import_tasks: path_is_checked.yml

# Test operation of the 'state' parameter
- block:
- include_tasks: remove_links.yml
- include_tasks: tests_state.yml

# Cleanup
always:
- include_tasks: remove_links.yml

Expand All @@ -62,6 +68,7 @@
path: '/usr/bin/dummy{{ item }}'
state: absent
with_sequence: start=1 end=4

# *Disable tests on Fedora 24*
# Shippable Fedora 24 image provides chkconfig-1.7-2.fc24.x86_64 but not the
# latest available version (chkconfig-1.8-1.fc24.x86_64). update-alternatives
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,6 @@
path: '{{ item }}'
state: absent
with_items:
- "{{ alternatives_dir }}/dummy"
- /etc/alternatives/dummy
- /usr/bin/dummy
71 changes: 71 additions & 0 deletions tests/integration/targets/alternatives/tasks/tests_state.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
# Add a few dummy alternatives with state = present and make sure that the
# group is in 'auto' mode and the highest priority alternative is selected.
- name: Add some dummy alternatives with state = present
alternatives:
name: dummy
path: "/usr/bin/dummy{{ item.n }}"
link: /usr/bin/dummy
priority: "{{ item.priority }}"
state: present
loop:
- { n: 1, priority: 50 }
- { n: 2, priority: 70 }
- { n: 3, priority: 25 }

- name: Ensure that the link group is in auto mode
felixfontein marked this conversation as resolved.
Show resolved Hide resolved
shell: 'head -n1 {{ alternatives_dir }}/dummy | grep "^auto$"'

# Execute current selected 'dummy' and ensure it's the alternative we expect
- name: Execute the current dummy command
shell: dummy
register: cmd

- name: Ensure that the expected command was executed
assert:
that:
- cmd.stdout == "dummy2"

# Add another alternative with state = 'selected' and make sure that
# this change results in the group being set to manual mode, and the
# new alternative being the selected one.
- name: Add another dummy alternative with state = selected
alternatives:
name: dummy
path: /usr/bin/dummy4
link: /usr/bin/dummy
priority: 10
state: selected

- name: Ensure that the link group is in manual mode
shell: 'head -n1 {{ alternatives_dir }}/dummy | grep "^manual$"'

- name: Execute the current dummy command
shell: dummy
register: cmd

- name: Ensure that the expected command was executed
assert:
that:
- cmd.stdout == "dummy4"

# Set the currently selected alternative to state = 'present' (was previously
# selected), and ensure that this results in the group being set to 'auto'
# mode, and the highest priority alternative is selected.
- name: Set current selected dummy to state = present
alternatives:
name: dummy
path: /usr/bin/dummy4
link: /usr/bin/dummy
state: present

- name: Ensure that the link group is in auto mode
shell: 'head -n1 {{ alternatives_dir }}/dummy | grep "^auto$"'

- name: Execute the current dummy command
shell: dummy
register: cmd

- name: Ensure that the expected command was executed
assert:
that:
- cmd.stdout == "dummy2"