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
Prev Previous commit
Next Next commit
fix for Python 2.7 compatibility
  • Loading branch information
tprestegard committed Apr 25, 2022
commit 45fe43619657c434a38da1f52f4fda15512d0d2f
14 changes: 10 additions & 4 deletions plugins/modules/system/alternatives.py
Original file line number Diff line number Diff line change
Expand Up @@ -80,18 +80,21 @@
state: present
'''

import enum
import os
import re
import subprocess

from ansible.module_utils.basic import AnsibleModule


class AlternativeState(str, enum.Enum):
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():

Expand All @@ -101,8 +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=[e.value for e in AlternativeState],
default=AlternativeState.SELECTED),
state=dict(
type='str',
choices=AlternativeState.to_list(),
default=AlternativeState.SELECTED,
),
),
supports_check_mode=True,
)
Expand Down