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.

Sign up for GitHub

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

Jump to bottom

Add VMware transport #321

Merged
merged 2 commits into from
Jul 12, 2018
Merged

Add VMware transport #321

merged 2 commits into from
Jul 12, 2018

Conversation

jerryaldrichiii
Copy link
Contributor

This adds the VMware transport via PowerCLI.

See: inspec/inspec#3133

@jerryaldrichiii
Add VMware transport
3a4be64 
Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>
clintoncwolfe
clintoncwolfe requested changes Jul 11, 2018
View reviewed changes
Copy link
Contributor

@clintoncwolfe clintoncwolfe left a comment

Choose a reason for hiding this comment

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

Only major change is a request to update the README, and consider the transport's placement in the platform tree.

lib/train/platforms/detect/specifications/api.rb
@@ -11,6 +11,8 @@ def self.load
plat.name('aws').in_family('cloud')
plat.name('azure').in_family('cloud')
plat.name('gcp').in_family('cloud')
plat.name('vmware').in_family('cloud')
Copy link
Contributor

Choose a reason for hiding this comment

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

Methinks vmware might be a member of iaas, not cloud. But that may be a marketing distinction, here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, I struggled with this as well. After discussing, @jquick and @chris-rock decided that is should be under cloud.

test/unit/transports/vmware_test.rb
stub_options: {
powershell_binary: :powershell
}
).connection
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see a test assertion in this block - is that intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I expect it to create a Local::Connection if the PowerShell binary is proper PowerShell (vs pwsh).

So I did an expect, then created the connection. Is there a better way?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh, I see. the expects should fire. Too many testing paradigms on this project :-)

test/unit/transports/vmware_test.rb
)
}
).connection
}.must_raise(RuntimeError)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a more specific exception class you could catch? You're testing a very specific circumstance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm...should I raise a different error? My plan was to ensure it raises, capture the exception in e then make sure the message is what I expect.

lib/train/transports/vmware.rb
def initialize(options)
super(options)

options[:viserver] = options[:viserver] || options[:host]
Copy link
Contributor

Choose a reason for hiding this comment

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

Fun shorthand: try ||=. Like +=, but for or. It's great for setting defaults.

options[:viserver] ||= options[:host]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed it is, will do!

lib/train/transports/vmware.rb
end

def local?
true
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this constant?

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 think so, I'm trying to override:

train/lib/train/plugins/base_connection.rb

Line 76 in d7b958c

def local?

lib/train/transports/vmware.rb
@@ -0,0 +1,190 @@
# encoding: utf-8
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a lot going on here that I really wish we had integration tests for. The unit tests are solid - I believe that the accessors and option parsers work. I'm sure info about integration testing is coming, likely in the resource pack.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, I too would like integration tests, I'm just not quite sure how to approach it here.

lib/train/transports/vmware.rb
@@ -0,0 +1,190 @@
# encoding: utf-8
Copy link
Contributor

Choose a reason for hiding this comment

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

That said, pleas update the README.md to include vmware, and provide configuration information there.

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've added sections covering basic usage. Detailed info (such as PWSH/PowerCLI installation will be in the resource pack).

Does that work for you? 😄

@jerryaldrichiii
Add VMware info to README.md
d32d887 
Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>
clintoncwolfe
clintoncwolfe approved these changes Jul 12, 2018
View reviewed changes
Copy link
Contributor

@clintoncwolfe clintoncwolfe left a comment

Choose a reason for hiding this comment

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

Thanks @jerryaldrichiii

@clintoncwolfe clintoncwolfe merged commit befdb6d into master Jul 12, 2018
@clintoncwolfe clintoncwolfe deleted the ja/add-vmware-transport branch July 12, 2018 17:51
jerryaldrichiii added a commit to chef-boneyard/inspec-vmware that referenced this pull request Jul 12, 2018
@jerryaldrichiii
Convert repo to use InSpec's PowerCLI support
78dbeed 
Support for VMware via PowerCLI was added to InSpec/Train. See:

  - inspec/inspec#3195
  - inspec/train#321

This removes the current code in favor of using InSpec's PowerCLI
implementation. Currently, the only supported resource is
`powercli_command`, but future resources are planned.

If you would like to continue using the old method, you can access it
via GitHub's tagging mechanism.

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>
@jerryaldrichiii jerryaldrichiii mentioned this pull request Jul 12, 2018
Merged
jerryaldrichiii added a commit to chef-boneyard/inspec-vmware that referenced this pull request Jul 18, 2018
@jerryaldrichiii
Convert repo to use InSpec's PowerCLI support
f12f579 
Support for VMware via PowerCLI was added to InSpec/Train. See:

  - inspec/inspec#3195
  - inspec/train#321

This removes the current code in favor of using InSpec's PowerCLI
implementation. Currently, the only supported resource is
`powercli_command`, but future resources are planned.

If you would like to continue using the old method, you can access it
via GitHub's tagging mechanism.

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>
chris-rock pushed a commit to chef-boneyard/inspec-vmware that referenced this pull request Jul 19, 2018
@jerryaldrichiii @chris-rock
Convert repo to use InSpec's PowerCLI support (#23)
febc078 
* Update version for pre-migration tagging

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

* Convert repo to use InSpec's PowerCLI support

Support for VMware via PowerCLI was added to InSpec/Train. See:

  - inspec/inspec#3195
  - inspec/train#321

This removes the current code in favor of using InSpec's PowerCLI
implementation. Currently, the only supported resource is
`powercli_command`, but future resources are planned.

If you would like to continue using the old method, you can access it
via GitHub's tagging mechanism.

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>

* Respond to feedback

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clintoncwolfe clintoncwolfe clintoncwolfe approved these changes

@miah miah miah approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jerryaldrichiii @miah @clintoncwolfe