Move subp into its own module.#416
Conversation
|
looking at this diff makes me think we should extract the service handling code into its own set of functions ooooooor, move them into distro 😅 |
This was painful, but it finishes a TODO from cloudinit/subp.py. It moves the following from util to subp: ProcessExecutionError subp which target_path I moved subp_blob_in_tempfile into cc_chef, which is its only caller. That saved us from having to deal with it using write_file and temp_utils from subp (which does not import any cloudinit things now). It is arguable that 'target_path' could be moved to a 'path_utils' or something, but in order to use it from subp and also from utils, we had to get it out of utils.
Add myself to list of those that have signed cla.
| return None | ||
|
|
||
|
|
||
| def is_exe(fpath): |
There was a problem hiding this comment.
Can we make this private since it's only a helper function used locally which is not intended for external consumers?
def _is_exe(fpath)?
I also wonder about pulling over runparts into cloudinit.subp too and actually have it call _is_exe instead of the duplicate logic " if os.path.isfile(exe_path) and os.access(exe_path, os.X_OK): "
blackboxsw
left a comment
blackboxsw
left a comment
There was a problem hiding this comment.
Looks good, three somewhat minor notes.
- is_exe -> _is_exe
- Can we move util.runparts -> subp.runparts
- subp.runparts to call _is_exe too
I think I agree with all of these, but suggest that maybe they are all sane as follow-on. Thoughts? |
OK, I went ahead and did this one. |
smoser
force-pushed
the
feature/move-subp
branch
2 times, most recently
from
e704e4d to
e977ff8
Compare
Yes let's pull in parts 2 & 3 as a separate PR. |
blackboxsw
left a comment
blackboxsw
left a comment
There was a problem hiding this comment.
Thanks for this,
Let's move runparts out of util as a separate exercise so we also can leverage is_exe from subp.subp (and thin out function definitions in util).
This was brought up in review of canonical#416. Makes sense to remove the local copy of "is this executable file".
runparts (run a directory of scripts) seems to fit well in subp module. The request to move it there was raised in canonical#416.
runparts (run a directory of scripts) seems to fit well in subp module. The request to move it there was raised in canonical#416.
runparts (run a directory of scripts) seems to fit well in subp module. The request to move it there was raised in canonical#416. Replace use of logexc with LOG.debug as logexc comes from util.
runparts (run a directory of scripts) seems to fit well in subp module. The request to move it there was raised in #416. Replace use of logexc with LOG.debug as logexc comes from util.
This was brought up in review of canonical#416. Makes sense to remove the local copy of "is this executable file".
This was brought up in review of #416. Makes sense to remove the local copy of "is this executable file".
3 participants
This was painful, but it finishes a TODO from cloudinit/subp.py.
It moves the following from util to subp:
ProcessExecutionError
subp_blob_in_tempfile
subp
which
target_path
It is arguable that 'target_path' could be moved to a 'path_utils' or
something, but in order to use it from subp and also from utils,
we had to get it out of utils.