Add function systemd::systemd_escape #243
Conversation
jkroepke
force-pushed
the
escape
branch
7 times, most recently
from
fdbba7e to
f1f0c42
Compare
|
|
||
| def exec_systemd(*args) | ||
| exec_args = { failonfail: true, combine: false } | ||
| escaped = Puppet::Util::Execution.execute(['systemd-escape', args], **exec_args) |
There was a problem hiding this comment.
Relying on an external command should be avoided. Can't we just improve the current systemd::escape function if it's current behavior is wrong?
There was a problem hiding this comment.
See and use #242 to dicuss this. From my point of view its not worth to re-implement the complex logic from systemd-escape. Can you explain, why external commands should be avoided? Is there a documented guideline?
There was a problem hiding this comment.
My personal opinion: Calling out to external tools should always be the last option. But Puppet already does that to systemd. Now it would be nice if there would be a systemd API binding for Ruby, but that doesn't really exist and that would require us to install a gem, which also sucks. We can keep the old function as well, so people can choose between using systemd::systemd_escape vs systemd::escape. I think that's fine.
There was a problem hiding this comment.
so people can choose between
This was my first intension, too. And thats why I decide to use a different name. In combination with Defferred functions, systemd-escape is executed on the agent, not on the master.
I understand, calling an external tool is slow, but the puppet lange does not provide functions to translate characters to hex which is required to improve the existing function.
There was a problem hiding this comment.
The hex translation could be handled by another function/it could be rewritten as a Ruby function, but IMO that only makes it more complicated and there is no guarantee that our reimplementation works like the actual systemd-escape. That's why I think having both implementations is fine.
#242 is here to discuss this. This PR is a proposal for one solution. |
| # @param path Use path (-p) ornon-path style escaping. | ||
| dispatch :escape do | ||
| param 'String', :input | ||
| optional_param 'Optional[Boolean]', :path |
There was a problem hiding this comment.
I think optional booleans are odd. Can't we simply have a boolean with a default false? Or put in a different way: what's the difference here between nil / undef and false?
There was a problem hiding this comment.
I copy this from here https://github.com/puppetlabs/puppetlabs-stdlib/blob/276e7b7cd97580c9c08b264b3727e5902df1f487/lib/puppet/functions/to_json_pretty.rb#L48 where Optional[Boolean] is defined, but the default value is false (like line 14 here).
|
Whats next here? Planned to merge the PR? Otherwise, I will close the stale PR. |
Co-authored-by: Ewoud Kohl van Wijngaarden <[email protected]>
|
Rebased. Test reduced. removed robocup comments. |
|
@smortex Are you fine with the latest changes? |
bastelfreak
changed the title
This PR is a proposal for #242 and its intended to replace the current function
systemd::escape.Pull Request (PR) description
This PR add a function
systemd::systemd_escapebased on the system call systemd-escape.See #220 and #95
rubocop:disable Style/OptionalBooleanParameteris required here, otherwise puppet wont detect the optional parameter.This Pull Request (PR) fixes the following issues
Fixes #242