-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Implement #9650 Add parameter hooks to inventory plugin iocage #9651
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution!
I'm wondering a bit about the term hook
. Is this some nomenclature that's used in the iocage
surroundings for such things? If not, maybe another name would be better. I'm not very good at coming up with names, but maybe something like read_file_contents
, resulting in iocage_file_contents
being set?
p = Popen(cmd_cat_hook, stdout=PIPE, stderr=PIPE, env=my_env) | ||
stdout, stderr = p.communicate() | ||
if p.returncode != 0: | ||
iocage_hooks.append('-') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a warning should be printed in this case? (Or the behavior should be configurable - ignore, warn, error.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The jails may be heterogeneous, and a hook that works for one jail may not work for the other. I want to keep the spirit of silently ignoring No such file
or any other error:
-
I don't want to complicate the use case where different jails use different hooks or no hooks at all. List all hooks and let the compose option pick what is needed.
-
The admins should be responsible for
intercepting
anything. And they should be used to it, especially in the case of thehooks
. For example, the /etc/dhclient-enter-hooks and /etc/dhclient-exit-hooks silently ignore any failing lines in the scripts. It is expected, that the admin is responsible for checking what a hook is doing. There are also security implications.
We can add the options (ignore, warn, error) later.
raise AnsibleError(f'Invalid (non unicode) input returned: {e}') from e | ||
|
||
except Exception: | ||
iocage_hooks.append('-') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
Also, why not using None
instead of -
? None
can never appear as a real value, so you can distinguish "error happened" from "-
was actually read".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dash "-" is used in iocage to represent a missing value. See for example ioc_list.py#L259 or ioc_list.py#L276. We've already used it too:
if iocage_ip4_dict['ip4']:
iocage_ip4 = ','.join([d['ip'] for d in iocage_ip4_dict['ip4']])
else:
iocage_ip4 = '-'
The nomenclature is general. A In particular, dhclient-script says:
There is no better name. |
Co-authored-by: Felix Fontein <[email protected]>
But generally hooks are running code. I would expect to be able to provide a script that runs something when reading "hook". So I think the name "hook" is not a good choice in this context. |
I see. I renamed the parameter to hooks_results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM as far as I can tell.
One thing I noticed, but probably for a different PR, is that the YAML indentation is not consistent within the file (or with other plugins for that matter).
Thank you for the review!
shell> ansible-doc -t inventory community.general.iocage
...
keyed_groups Add hosts to group based on the values of a variable.
default: []
elements: dict
type: list
suboptions:
default_value The default value when the host variable's value is an empty
string.
This option is mutually exclusive with
`keyed_groups[].trailing_separator'.
default: null
type: str
added in: version 2.12 of ansible-core
key The key from input dictionary used to generate groups.
default: null
... |
SUMMARY
Implement #9650 Add parameter hooks to inventory plugin iocage.
ISSUE TYPE
COMPONENT NAME
iocage.py
ADDITIONAL INFORMATION
Tested in Ubuntu 24.04