Skip to content
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

[SCVMM] Always assume a string for run_powershell_script #14859

Merged
merged 1 commit into from
Apr 25, 2017
Merged

[SCVMM] Always assume a string for run_powershell_script #14859

merged 1 commit into from
Apr 25, 2017

Conversation

djberg96
Copy link
Contributor

This fixes an issue where our run_powershell_script method was assuming a file argument, but in most cases a string (heredoc) argument was being passed instead.

This PR alters the code so that it's always assumes a string instead of a file.

https://bugzilla.redhat.com/show_bug.cgi?id=1444201

@djberg96 djberg96 changed the title Always assume a string for run_powershell_script [SCVMM] Always assume a string for run_powershell_script Apr 24, 2017
@djberg96
Copy link
Contributor Author

@miq-bot add_label fine/yes

@djberg96
Copy link
Contributor Author

@miq-bot add_label euwe/no

@djberg96
Copy link
Contributor Author

@miq-bot add_label darga/no

@jerryk55
Copy link
Member

LGTM, not that anybody asked. :-)

@djberg96
Copy link
Contributor Author

@jerryk55 Appreciated!

@jteehan
Copy link

jteehan commented Apr 24, 2017

Verified against 5.8.0.11 and the ERR message is no longer present. This fix is working fine.

@@ -20,7 +20,8 @@ def ems_inv_to_hashes
log_header = "MIQ(#{self.class.name}.#{__method__}) Collecting data for EMS name: [#{@ems.name}] id: [#{@ems.id}]"
$scvmm_log.info("#{log_header}...")

@inventory = ManageIQ::Providers::Microsoft::InfraManager.execute_powershell_json(@connection, INVENTORY_SCRIPT)
@script = IO.read(INVENTORY_SCRIPT)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this have to be an instance var? Looks like it's just used on the next line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figured I would do that just in case we needed it elsewhere someday.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@blomquisg ok, changed it. :)

@bronaghs
Copy link

@djberg96 - what caused this to break all of a sudden?

@djberg96
Copy link
Contributor Author

@bronaghs I think I broke something in the process of cleaning up some things while I was working on the refresh parser revamp.

Added some basic run_powershell_script specs.
@miq-bot
Copy link
Member

miq-bot commented Apr 25, 2017

Checked commit https://github.com/djberg96/manageiq/commit/4337c139afbcdf729e6a9f8c1a67c335e1fca658 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 4 offenses detected

spec/models/manageiq/providers/microsoft/infra_manager/powershell_spec.rb

@blomquisg blomquisg merged commit a3cafd8 into ManageIQ:master Apr 25, 2017
@blomquisg blomquisg added this to the Sprint 60 Ending May 8, 2017 milestone Apr 25, 2017
simaishi pushed a commit that referenced this pull request Apr 26, 2017
[SCVMM] Always assume a string for run_powershell_script
(cherry picked from commit a3cafd8)

https://bugzilla.redhat.com/show_bug.cgi?id=1445936
@simaishi
Copy link
Contributor

simaishi commented Apr 26, 2017

Fine backport details:

$ git log -1
commit 5c356b524ca1da820084446c4f7e356f6f192cf0
Author: Greg Blomquist <blomquisg@gmail.com>
Date:   Tue Apr 25 10:58:01 2017 -0400

    Merge pull request #14859 from djberg96/powershell2
    
    [SCVMM] Always assume a string for run_powershell_script
    (cherry picked from commit a3cafd81b6d1d579343374a32e2c3078e16d7816)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1445936

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants