Remove MiqUtil.runcmd in favor of AwesomeSpawn #510
Conversation
| end | ||
|
|
||
| def self.runcmd_with_logging(cmd_str, opts, params = {}) | ||
| def self.run_command_with_logging(cmd_str, opts, params = {}) |
There was a problem hiding this comment.
Similarly here I renamed it for clarity. There are exactly 2 callers in core specs that stub runcmd_with_logging, which is why i need the alias below, but I'll remove that in a follow up.
| @@ -28,7 +28,7 @@ def self.execute(ps_command) | |||
| ps_exec = exepath | |||
| command = "start /wait cmd /c #{ps_exec} #{ps_command}" | |||
| $log.debug "PowerShell: Running command: [#{command}]" if $log | |||
| ret = MiqUtil.runcmd(command).chomp | |||
| ret = AwesomeSpawn.run!(command, :combined_output => true).output.chomp | |||
There was a problem hiding this comment.
This one (and the next one) are super-interesting, because there is no way for them to possibly work. start and taskkill are Windows commands, and so can't be run by AwesomeSpawn nor could they have been run by MiqUtil.runcmd. I converted it for now, but it's actually broken. I think the correct way to run it is with WMIHelper.connectServer.runProcess(command, true), but I'm not 100% sure and I have no idea how to test it.
There was a problem hiding this comment.
I believe this code was used a long time ago when we had agents on clients for various reasons.
Searching for MiqPowerShell yields 3 results, only one of which is an actual caller. So, I believe we will be able to delete (in a future PR?) most of the guts of MiqPowerShell. If we try really hard, we can probably delete the whole module as the only remaining use for it is to convert strings. ;-)
There was a problem hiding this comment.
Yeah, I'm not certain we really need it anymore either...hoping we can kill it in a future PR.
| result = err.result | ||
| return err.to_s if continue_on_error == true && result.exit_status == 1 | ||
|
|
||
| raise "Command:<#{result.command_line}> exited with status:<#{result.exit_status}>\nCommand output:\n#{err}" |
There was a problem hiding this comment.
Also note this whole error handling was a bunch of bugs beforehand. command_line standalone is an non-existent variable name, and $? was never set by runcmd
|
This pull request is not mergeable. Please rebase and repush. |
|
Checked commit Fryguy@729d5c0 with ruby 2.6.3, rubocop 0.82.0, haml-lint 0.35.0, and yamllint lib/gems/pending/util/win32/miq-powershell.rb
|
|
😍 @Fryguy I didn't even know this was being done! 100% approve making this change, and is a big tech debt win! 🎉 |
Part of #231
Depends on:
@chessbyte Please review