Major change with multiple features #91
Conversation
If the site already exists, the site_id wasn't getting set in IIS.
This existed on 1/4 calls - just adding to the rest. This prevents an issue on IIS7, which doesn't like forward slashes in this arg.
Update README with :config action for both iis_site and iis_pool, :recycle action for iis_pool, and pool_username and pool_password attributes for iis_pool. No new functionality; documentation update only.
Added monospacing for some attributes and actions.
Converge the site_id during :config actions
Added chefspec matchers
Add win_friendly_path to all appcmd.exe /physicalPath arguments.
Fixing new_resource action being updated properly if action is called.
updated to fix typo
updated to fix bug with command
Conflicts: metadata.rb providers/pool.rb
|
Paging @adamedx, @btm, @jdmundrawala and @jtimberman. Gentleman, any way you can help Justin to get this merged? |
|
I'm not really qualified to comment about this with regard to IIS. However, I'll make some general comments in the code. |
| # Cookbook Name:: iis | ||
| # Resource:: lock | ||
| # | ||
| # Copyright:: 2011, Webtrends Inc. |
There was a problem hiding this comment.
Is Justin with Webtrends? Was this code written in 2011? The copyright should reflect reality.
There was a problem hiding this comment.
yeah, simple copy + paste remnant changed to be no Copyright
|
Generally speaking, we prefer to avoid really large changes like this that add a bunch of different functionality. It is easier to review smaller sets of changes. We also want to have consistent style in the code, which is why we use rubocop and foodcritic across cookbook projects. It would be superb to start getting test coverage for these new features, too, to ensure they "DWIM" and they don't alter existing functionality, especially where large amounts of code are refactored/moved around. |
| def initialize(*args) | ||
| super | ||
| @action = :config | ||
| end |
There was a problem hiding this comment.
I think we prefer the resource helper method, "default_action" here instead of an initialize method. The end result is the same, but the helper is easier to understand the intention.
There was a problem hiding this comment.
changed to use the default_action method
| identity_type = XPath.first(doc.root, "APPPOOL/add/processModel/@identityType").to_s == "SpecificUser" ? false : true | ||
| user_name = XPath.first(doc.root, "APPPOOL/add/processModel/@userName").to_s == @new_resource.pool_username.to_s || @new_resource.pool_username.to_s == '' ? false : true | ||
| password = XPath.first(doc.root, "APPPOOL/add/processModel/@password").to_s == @new_resource.pool_password.to_s || @new_resource.pool_password.to_s == '' ? false : true | ||
| end |
There was a problem hiding this comment.
These XPath lines could use some simplification or clarification. There's a bunch of order happening in there... looking at the right I might think that that the values on the left like user_name is either true or false. Some parenthesis might clear that up, but I there's probably a ton of value in considering if you could have a separate method implement this logic and then call that for each attribute/variable here.
Yeah, I'm just not sure what's happening here, so it could use some cleanup.
There was a problem hiding this comment.
looking down below it looks like these are just true/false values, not an actual value, so I'd recommend changing the variable names to something that indicidates that you're a) setting something and b) is a true/false value (end with?)
set_max_processes?
enable_32_bit_app_on_win_64?
set_managed_runtime_version?
Or whatever.
There was a problem hiding this comment.
added helper function and then added ? to variable names
|
It would be nice to update "ie vdirs" to "i.e. vdirs" in the README.md if you go back in there for anything. |
|
okay, I think this looks pretty good. it's a lot, which means we've just got to get it in and test it a bit before we release. we'll get it into master soon, and I hope you'll help test from master as this will be a major release. I'd really like to get the test-kitchen framework we recently got into the windows cookbook copied over here so we can create some integration tests for this. I'd love it if you'd help with building those out. 👍 |
|
@btm sounds good, I look forward to seeing this move forward as a major release. A lot of testing will need to be done both manually, and i'd love to be involved in adding the test-kitchen frameworks for IIS. Looking at the windows cookbook I don't see this being a huge issue, just a slight learning curve. |
|
Awesome, I hope to see this released soon. |
|
Hi Guys - im relatively new to chef - i have a cookbook which uses the iis cookbook(2.1.2). I really like the prospect of having the virtual directory resource and also the lock/unlock thats in this pull request. How long approximately will it be before this will be released? Will it be weeks or months. Also, need i wait for it to be released? i did a quick test - pulled down the latest iis and windows cookbook and i got an error when i ran my cookbook which depends upon them. |
|
@endakenny it could be weeks or months, depending how many people have time to work on it. If you've copied this cookbook out of git and tried using it and gotten errors, you can file them as github issues and someone with more experience can look at them. Just make sure you mention that you're running from the git master in each issue you file, and include a copy of the error. You can use three backticks to quote code on github like this: turns into this: |
|
@drpebcak if you could join the testing effort and submit issues, you can definitely speed up the release. @endakenny please submit your cookbook code and the error, i'll happily take a gander at it. |
|
@EasyAsABC123 I definitely will. I've already been using a slightly hacked together version of this cookbook to get certain things working, so I would love to help test these changes. |
|
@EasyAsABC123 - my cookbook is too big(rather than complex) to show you as a sample. What ive done is broken down the sample to your code above - with only the iis_pool and iis_site resources. Obviously i added a dependency on the iis cookbook. Then i ran in chef-client in local mode Here were the cookbooks used Here was the compile exception:
Recipe Compile Error in C:/Users/kennye/.chef/local-mode-cache/cache/cookbooks/iis/providers/app.rbTypeErrorwrong argument type Class (expected Module) Cookbook Trace:C:/Users/kennye/.chef/local-mode-cache/cache/cookbooks/iis/providers/app.rb:26:in Relevant File Content:C:/Users/kennye/.chef/local-mode-cache/cache/cookbooks/iis/providers/app.rb: 19: # limitations under the License. |
|
@EasyAsABC123 - Thank you - ive logged the issue on Github - The version of iis cookbook ive used up to now was 2.1.2 - not in production but had used it extensively on preproduction servers. |
|
@btm Chef::Util::PathHelper.validate_path doesn't seem to work with network paths, continually stating they are invalid. |
|
@btm all resolved except for the |
|
@btm nvm it was the usage of |
I apologize for the large pull request but the speed of approvals is taking too long and we have had to write too much code.