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

Add iis_site resource #907

Merged
merged 3 commits into from
Aug 17, 2016
Merged

Add iis_site resource #907

merged 3 commits into from
Aug 17, 2016

Conversation

chrisevett
Copy link
Contributor

I modeled this after serverspec and then added a check for bindings. The janky format we check for (you'll notice some string mangling) matches the output that gets generated by the get-website cmdlet in powershell. I thought that would be the easiest way to write tests.

# <protocol> <bindingInformation> {if https "sslFlags=<sslFlags>"}
# the binding is a collection and there will always be at least one entry per site
if site['bindings']['Collection']
site['bindings']['Collection'].each do |binding|
Copy link
Contributor

Choose a reason for hiding this comment

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

It may be better to use http://ruby-doc.org/core-2.2.0/Array.html#method-i-map for that use case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chris-rock was something like this what you meant?

 site['bindings']['Collection'].map { |k, str|  
                                        k['protocol'] << 
                                        " " << 
                                        k['bindingInformation'] <<
                                        (k['protocol'] == 'https' ? ' sslFlags=' << flags : '')
                                      }

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah!

Copy link
Contributor

Choose a reason for hiding this comment

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

The idea was to remove the bindings_array = [] object

@chris-rock
Copy link
Contributor

chris-rock commented Aug 11, 2016

@chrisevett Great effort, this is a great resource. Let just make some improvements. Compared to the original http://serverspec.org/resource_types.html#iis_website it is easier to read!

Let me know if something is unclear.

@chrisevett
Copy link
Contributor Author

@chris-rock I'll make the updates you suggested. Thanks for the feedback.

@chris-rock
Copy link
Contributor

Hey @chrisevett Please let me know when you think its ready, I am going to review it then again

@@ -5,7 +5,7 @@
description 'This cookbook prepares the test operating systems'
version '1.0.0'

depends 'apt'
depends 'iis'
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 any reason you removed the apt cookbook?

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 undid this change.

@chrisevett
Copy link
Contributor Author

@chris-rock
This is good to go. In the interest of cutting down the clutter would it be better to smoosh this into one commit or would you rather merge it as is so we keep the history?

Thanks for being patient with my poor ruby skills.

@chris-rock
Copy link
Contributor

@chrisevett it would be great if you could squash that to no more that 5 commits. git rebase -i master is a good tool for that.


return unless node['platform_family'] == 'windows'

# make sure the iis windows feature is installed
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we do not need the indent 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.

I removed the indent.

@chrisevett
Copy link
Contributor Author

@chris-rock this is ready for review once more. Thanks!


* ``iis_site()`` must specify a web site name
* ``'website'`` is the web site name
* ``'service_name'`` is a service listed in the ``inetd.conf`` file
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we do not need the service_name here

@chef-supermarket
Copy link

Hi. I am an automated pull request bot named Curry. There are commits in this pull request whose authors are not yet authorized to contribute to Chef Software, Inc. projects or are using a non-GitHub verified email address. To become authorized to contribute, you will need to sign the Contributor License Agreement (CLA) as an individual or on behalf of your company. You can read more on Chef's blog.

Non-GitHub Verified Committers

There are 1 commit author(s) whose commits are authored by a non-GitHub verified email address. Chef will have to manually verify that they are authorized to contribute.

Please sign the CLA here.

@chris-rock
Copy link
Contributor

Awesome work @chrisevett The resource works great 👍 I'll added some super minor improvements. Then lets just rebase this PR to latest master via git rebase master

@chris-rock
Copy link
Contributor

I'll merge this PR as soon as you're done

@chef-supermarket
Copy link

Hi. Your friendly Curry bot here. Just letting you know that all commit authors have become authorized to contribute. I have added the "Signed CLA" label to this issue so it can easily be found in the future.

@chrisevett
Copy link
Contributor Author

Rebase and edits are complete. Thanks.

@chris-rock
Copy link
Contributor

Thanks again for hard work @chrisevett 💯

@chris-rock chris-rock merged commit 775d41a into inspec:master Aug 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants