jenkins_node: Add launch_ssh option and suboptions #9101
Conversation
* Implement launch_ssh option which will configure a node to use the SSH launcher method if specified. * Implement common suboptions for launch_ssh option, notably the various SSH host verification strategies. * Establish a pattern for dealing with non-trivial XML in this module - ...Element wrappers for handling structural XML and ...Config for idempotent application of configuration.
ansibullbot
added
feature
felixfontein
added
check-before-release
changelogs/fragments/9101-jenkins_node-add-launch_ssh-option.yaml
Outdated
Show resolved
Hide resolved
Co-authored-by: Felix Fontein <[email protected]>
Co-authored-by: Felix Fontein <[email protected]>
Co-authored-by: Felix Fontein <[email protected]>
Co-authored-by: Felix Fontein <[email protected]>
Co-authored-by: Felix Fontein <[email protected]>
| def bool_to_text(value): # type: (bool) -> str | ||
| return "true" if value else "false" | ||
|
|
||
|
|
||
| def text_to_bool(text): # type: (str) -> bool | ||
| if text == "true": | ||
| return True | ||
|
|
||
| if text == "false": | ||
| return False | ||
|
|
||
| raise ValueError("unexpected boolean text value '{}'".format(text)) |
There was a problem hiding this comment.
I cannot easily check this right now but I fairly sure that something like that already exists within the Ansible codebase - if not here in c.g., maybe in ansible-core itself. Worth a check.
| choices: | ||
| - true |
There was a problem hiding this comment.
Any particular reason to add this? Being a bool automatically restraints the accepted values.
There was a problem hiding this comment.
Good question.
This is an example of where there are N mutually exclusive options of which you may specify zero or one of them.
Some of the options have suboptions - these are either specified with a dict of suboptions or not specified at all.
The other options are flags - these are either specified with True or not specified at all.
I wanted to be explicitly about this by preventing setting False for these flags because it might be tempting to specify
d1: {...} # Selected option
f1: no
f2: no
which looks correct but will fail to validate.
There was a problem hiding this comment.
It is kind of a common usage pattern in Ansible to assume that bool options will handle false and null in the exact same way. If you still think you should keep the choices, then I would suggest you make an explicit mention of that in the description - to ensure users are warned about this being different (reinforcing the fact that "choices" will appear in the docs as well).
| if sys.version_info[0] <= 3 or sys.version_info[1] < 8: | ||
| class cached_property(object): # noqa | ||
| def __init__(self, func): | ||
| self.func = func | ||
|
|
||
| def __get__(self, instance, cls): | ||
| if instance is None: | ||
| return self | ||
|
|
||
| value = self.func(instance) | ||
| setattr(instance, self.func.__name__, value) | ||
|
|
||
| return value | ||
| else: | ||
| from functools import cached_property |
There was a problem hiding this comment.
I would suggest making it one notch more generic with:
| if sys.version_info[0] <= 3 or sys.version_info[1] < 8: | |
| class cached_property(object): # noqa | |
| def __init__(self, func): | |
| self.func = func | |
| def __get__(self, instance, cls): | |
| if instance is None: | |
| return self | |
| value = self.func(instance) | |
| setattr(instance, self.func.__name__, value) | |
| return value | |
| else: | |
| from functools import cached_property | |
| try: | |
| from functools import cached_property | |
| except ImportError: | |
| class cached_property(object): # noqa | |
| def __init__(self, func): | |
| self.func = func | |
| def __get__(self, instance, cls): | |
| if instance is None: | |
| return self | |
| value = self.func(instance) | |
| setattr(instance, self.func.__name__, value) | |
| return value |
| @abstractmethod | ||
| def init(self): # type: () -> et.Element | ||
| """Initialize XML element from config. | ||
|
|
||
| Returns: | ||
| Initialized XML element. | ||
| """ | ||
| raise NotImplementedError |
There was a problem hiding this comment.
The decorator is not really necessary if all the method does is raising NotImplementedError. It's not wrong, but it is redundant.
There was a problem hiding this comment.
Good point - I had wanted to use it to prevent instantiating instances of classes with unimplemented methods but I couldn't be bothered faffing about with a 2 + 3 compatible ABC/ABCMeta.
I will remove it.
| TEMPLATE = """ | ||
| <sshHostKeyVerificationStrategy class="hudson.plugins.sshslaves.verifiers.ManuallyTrustedKeyVerificationStrategy"> | ||
| <requireInitialManualTrust>false</requireInitialManualTrust> | ||
| </sshHostKeyVerificationStrategy> | ||
| """.strip() |
There was a problem hiding this comment.
Another way to do this, without having to call .strip() is:
| TEMPLATE = """ | |
| <sshHostKeyVerificationStrategy class="hudson.plugins.sshslaves.verifiers.ManuallyTrustedKeyVerificationStrategy"> | |
| <requireInitialManualTrust>false</requireInitialManualTrust> | |
| </sshHostKeyVerificationStrategy> | |
| """.strip() | |
| TEMPLATE = ( | |
| "<sshHostKeyVerificationStrategy class="hudson.plugins.sshslaves.verifiers.ManuallyTrustedKeyVerificationStrategy">\n" | |
| " <requireInitialManualTrust>false</requireInitialManualTrust>\n" | |
| "</sshHostKeyVerificationStrategy>" | |
| ) |
Maybe one notch "cleaner", though it seems this TEMPLATE is only passed on to et.fromstring() for parsing - in which case the newlines at the end should probably not matter at all.
There was a problem hiding this comment.
Another way to handle that could be:
| TEMPLATE = """ | |
| <sshHostKeyVerificationStrategy class="hudson.plugins.sshslaves.verifiers.ManuallyTrustedKeyVerificationStrategy"> | |
| <requireInitialManualTrust>false</requireInitialManualTrust> | |
| </sshHostKeyVerificationStrategy> | |
| """.strip() | |
| TEMPLATE = """\ | |
| <sshHostKeyVerificationStrategy class="hudson.plugins.sshslaves.verifiers.ManuallyTrustedKeyVerificationStrategy"> | |
| <requireInitialManualTrust>false</requireInitialManualTrust> | |
| </sshHostKeyVerificationStrategy>\ | |
| """ |
| @cached_property | ||
| def launch(self): # type: () -> LauncherConfig | None | ||
| if self.module.params["launch_ssh"]: | ||
| return ssh_launcher_args_config(self.module.params["launch_ssh"]) | ||
|
|
||
| return None |
There was a problem hiding this comment.
I think it would be simpler - and more consistent with existing code - to define self.launch in the __init__() method, like self.offline_message. By doing that you could remove the cached_property decorator entirely, also discarding the import check for Python 3.8.
| @@ -235,6 +699,10 @@ def configure_node(self, present): | |||
| data = self.instance.get_node_config(self.name) | |||
| root = et.fromstring(data) | |||
|
|
|||
| if self.launch is not None: | |||
There was a problem hiding this comment.
nit pickly, this could be made one notch simpler with:
| if self.launch is not None: | |
| if self.launch: |
There was a problem hiding this comment.
I've been wondering about this sort of situation recently and had come to the conclusion that it's more explicit and faster to do the identity check to constrain from T | None to None than invoke __bool__.
| host=dict(type='str'), | ||
| port=dict(type='int'), | ||
| credentials_id=dict(type='str'), | ||
| host_key_verify_none=dict(type='bool', choices=[True]), |
There was a problem hiding this comment.
Mentioned it before, but the choices really seem redundant here.
|
@phyrwork ping! needs_info |
ansibullbot
added
needs_info
Implement launch_ssh option which will configure a node to use the SSH launcher method if specified.
Implement common suboptions for launch_ssh option, notably the various SSH host verification strategies.
Establish a pattern for dealing with non-trivial XML in this module - ...Element wrappers for handling structural XML and ...Config for idempotent application of configuration.
ISSUE TYPE
COMPONENT NAME
jenkins_node