-
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. Just list all hooks and let the compose option pick what is needed.
-
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 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 if needed.
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".
The nomenclature is general. A In particular, dhclient-script says:
There is no better name. |
Co-authored-by: Felix Fontein <[email protected]>
SUMMARY
Implement #9650 Add parameter hooks to inventory plugin iocage.
ISSUE TYPE
COMPONENT NAME
iocage.py
ADDITIONAL INFORMATION
Tested in Ubuntu 24.04