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

Adds a 'clear' appcmd option #208

Merged
merged 1 commit into from
Oct 21, 2015

Conversation

spuder
Copy link
Contributor

@spuder spuder commented Sep 2, 2015

Adds the ability to remove a setting from a config file.

#207

@EasyAsABC123
Copy link
Contributor

👍 holy cow sorry I misunderstood what you were asking and speaking of. I re-read and yes this is perfectly fine and I understand what you are wanting here.

Your code will do this beautifully, we could do a little cleanup to make it cleaner.

I'll add comments to the code

@spuder
Copy link
Contributor Author

spuder commented Sep 2, 2015

Thanks for the quick feedback. I'm going to close this feature request, and move the discussion to the open issue #207

@spuder
Copy link
Contributor Author

spuder commented Sep 2, 2015

Thanks for the update. I'll look into making this into a function.

Once I get this working, I'm not really sure how to test this. Any suggestions welcome.

@EasyAsABC123
Copy link
Contributor

@spuder sent you a PR for your branch, this is just an example please don't feel like you need to accept it just edit yours to be like that.

@spuder
Copy link
Contributor Author

spuder commented Sep 2, 2015

Here is a commit that is slightly more squished and more functional.

1dc927e

  • Write Tests
  • Decide if :config should be replaced with :set in the README

@spuder
Copy link
Contributor Author

spuder commented Sep 2, 2015

Alright, think I got it. All squashed up here: 14e0b62

Note this also updates the README to remove :config and use :set as the default action.

@spuder
Copy link
Contributor Author

spuder commented Sep 2, 2015

Updates readme to show deprecation.

130ce0d

@EasyAsABC123
Copy link
Contributor

@spuder looks perfect! 👍

The change is minor (codewise) but i'd like another to review it. Probably need something in the test-kitchen branch

@spuder spuder changed the title WIP: Adds a clear config Adds a 'clear' appcmd option Sep 2, 2015
@spuder
Copy link
Contributor Author

spuder commented Sep 3, 2015

With this pull request, the following appcmd commands are supported.

  • appcmd set config
  • appcmd clear config

I got to thinking. There are other appcmd config options we could implement if we want. The only other one that I think would be somwhat useful would be reset.
lock and unlock are already implemented with a different provider.

�  list      List configuration sections
  set       Edit configuration section
  search    Find configuration paths where particular settings are defined
  lock      Lock the configuration section
  unlock    Unlock the configuration section
  clear     Clear the section configuration
  reset     Reset the section configuration to default values
  migrate   Migrate a legacy configuration section forward�����������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������

@EasyAsABC123
Copy link
Contributor

@spuder there is definitely a ton of features we need to implement, normally it is done on request of functionality.

But I understand your point

@EasyAsABC123
Copy link
Contributor

@spuder have you been able to test this? thanks

@spuder
Copy link
Contributor Author

spuder commented Sep 14, 2015

I am working on more extensive smoke tests. Will update with results.

@spuder
Copy link
Contributor Author

spuder commented Sep 16, 2015

I'm finding that there is a bug in microsoft's appcmd clear command.

  • If a key already exists, it will duplicate it with an empty xml section, rather than removing the content.
# create machine key
appcmd.exe set config /commit:WEBROOT /section:machineKey /validation:AES /validationKey:1234 /decryptionKey:5678
# So far so good
# c:\windows\microsoft.NET\framework64\v4.0.30319\Config\web.config
<machineKey decryption="AES" decryptionKey="5678" validation="AES" validationKey="1234" />
# clear machine key
appcmd clear config /commit:WEBROOT /section:machineKey
# appcmd clear creates a duplicate xml key, breaking IIS
# c:\windows\microsoft.NET\framework64\v4.0.30319\Config\web.config
<machineKey decryption="AES" decryptionKey="5678" validation="AES" validationKey="1234" />
<machineKey />

screenshot 2015-09-16 13 47 47

screenshot 2015-09-16 13 48 09

Same results if I give the full path

appcmd clear config /commit:WEBROOT /section:machineKey /validation:AES /validationKey:1234 /decryptionKey:5678

Not really sure where to go from here. If a bare appcmd won't work from powershell, it definitely won't work from chef. Maybe someone with more experience with IIS can point out the correct way to remove a setting. For now this is on hold until I figure this out.

@EasyAsABC123
Copy link
Contributor

So I just tested this and with minor tweaking to your commit path it works fine using the reset command

C:\Windows\System32\inetsrv>appcmd.exe set config /commit:MACHINE/WEBROOT/APPHOST /section:machineKey /validation:AES /validationKey:1234 /decryptionKey:5678
Applied configuration changes to section "system.web/machineKey" for
"MACHINE/WEBROOT/APPHOST" at configuration commit path "MACHINE/WEBROOT/APPHOST"

C:\Windows\System32\inetsrv>appcmd.exe reset config /commit:MACHINE/WEBROOT/APPHOST /section:machineKey
Cleared section "system.web/machineKey" at configuration path "MACHINE/WEBROOT/APPHOST"
Reset section "system.web/machineKey" to default values

@spuder
Copy link
Contributor Author

spuder commented Sep 17, 2015

Smoke test is successful.

PS C:\Windows\System32\inetsrv> ./appcmd.exe list config /commit:MACHINE/WEBROOT/APPHOST /section:machineKey
<system.web>
  <machineKey decryptionKey="5678" validation="AES" validationKey="1234" />
</system.web>����������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������
iis_config "/commit:MACHINE/WEBROOT/APPHOST /section:machineKey" do
  action :clear
end
PS C:\Windows\System32\inetsrv> chef-client -z -r "recipe[nd-web::clear_apcmd]" -c C:\chef\client.rb
[2015-09-17T17:56:21-04:00] INFO: Started chef-zero at chefzero://localhost:8889 with repository at c:/Users/sowen
  One version per cookbook
  data_bags at c:/chef/data_bags
  environments at c:/chef/chef-repo-lab/environments
  roles at c:/chef/chef-repo-lab/roles

Starting Chef Client, version 12.4.1
[2015-09-17T17:56:28-04:00] INFO: *** Chef 12.4.1 ***
[2015-09-17T17:56:28-04:00] INFO: Chef-client pid: 17928
[2015-09-17T17:56:37-04:00] INFO: Setting the run_list to [#<Chef::RunList::RunListItem:0x444d440 @version=nil, @type=:recipe, @name="nd-web::clear_apcmd">] from CLI options
[2015-09-17T17:56:37-04:00] INFO: Run List is [recipe[nd-web::clear_apcmd]]
[2015-09-17T17:56:37-04:00] INFO: Run List expands to [nd-web::clear_apcmd]
[2015-09-17T17:56:37-04:00] INFO: Starting Chef Run for spencerrole3
[2015-09-17T17:56:37-04:00] INFO: Running start handlers
[2015-09-17T17:56:37-04:00] INFO: Start handlers complete.
[2015-09-17T17:56:37-04:00] INFO: HTTP Request Returned 404 Not Found: Object not found:
resolving cookbooks for run list: ["nd-web::clear_apcmd"]
[2015-09-17T17:56:38-04:00] INFO: Loading cookbooks [nd-web@0.18.3, windows@1.38.1, iis@4.1.1, chef_handler@1.2.0]
Synchronizing Cookbooks:
  - windows
  - nd-web
  - iis
  - chef_handler
Compiling Cookbooks...
Converging 1 resources
Recipe: nd-web::clear_apcmd
  * iis_config[/commit:MACHINE/WEBROOT/APPHOST /section:machineKey] action clear[2015-09-17T17:56:39-04:00] INFO: Processing iis_config[/commit:MACHINE/WEBROOT/APPHOST /section:machineKey] action clear (nd-web::clear_apcmd line 16)
[2015-09-17T17:56:39-04:00] INFO: IIS Config command run


[2015-09-17T17:56:40-04:00] INFO: Chef Run complete in 2.734376 seconds

Running handlers:
[2015-09-17T17:56:40-04:00] INFO: Running report handlers
Running handlers complete
[2015-09-17T17:56:40-04:00] INFO: Report handlers complete
Chef Client finished, 1/1 resources updated in 18.953031 seconds���������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������

Verified by looking at appcmd list config and inspecting c:\windows\system32\inetsrv\config\applicationHost.config

PS C:\Windows\System32\inetsrv> ./appcmd.exe list config /commit:MACHINE/WEBROOT/APPHOST /section:machineKey
<system.web>
  <machineKey />
</system.web>����������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������

Also verified It is idempotent, and also works if the path doesn't exist (by removing configuration/system.web/machineKey from xml path in file)

@spuder
Copy link
Contributor Author

spuder commented Sep 17, 2015

Still needs to be documented. I will need to come up for an example since commit:MACHINE/WEBROOT/APPHOST /section:machineKey doesn't appear to be the right place for a machine key setting to go. (The key is not shown in the IIS managment console)

Squashed commits:
[4bb6e6e] Updates iis_config section in readme with better descriptions. (+1 squashed commit)
Squashed commits:
[130ce0d] Adds :config deprecation to readme (+1 squashed commit)
Squashed commits:
[891981f] Removes unneeded lines (+1 squashed commit)
Squashed commits:
[14e0b62] Updates readme of :clear (+1 squashed commit)
Squashed commits:
[a152a9a] Adds :set to action (+1 squashed commit)
Squashed commits:
[1dc927e] Converts clear to function (+1 squashed commit)
Squashed commits:
[dbf4552] Updates readme (+1 squashed commit)
Squashed commits:
[fa8f30a] Adds a clear config
@spuder
Copy link
Contributor Author

spuder commented Sep 17, 2015

Alright, got documentation. I think this pull request is 100% now.

EasyAsABC123 added a commit that referenced this pull request Oct 21, 2015
Adds a 'clear' appcmd option
worked for my limited smoke tests, merging to master.  Need test-kitchen script written for this
@EasyAsABC123 EasyAsABC123 merged commit a51cd41 into sous-chefs:master Oct 21, 2015
# Add static machine key at site level
iis_config "MySite /commit:site /section:machineKey /validation:AES /validationKey:AAAAAA /decryptionKey:ZZZZZ" do
action :config
end

Choose a reason for hiding this comment

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

Should this example us the new :set?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, this should be :set as that is the new command. Or simply no action at all since it is also the default.

Choose a reason for hiding this comment

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

With no action/default actuon we can drop the block too.

Copy link
Contributor

Choose a reason for hiding this comment

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

@philoserf definitely, for sample sake I decided to just leave the action even though it is unneeded. Updated it to :set for all examples

Choose a reason for hiding this comment

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

👍

@spuder spuder deleted the feature/clearconfig branch October 21, 2015 15:46
@spuder
Copy link
Contributor Author

spuder commented Oct 30, 2015

@EasyAsABC123 Any update when 4.1.2 or 4.2.0 will be released? I'm at the point where I'm ready for the clear command.

@EasyAsABC123
Copy link
Contributor

Hey Owen,
We really need test kitchen and Travis CI to stop failing so that we can have confidence in our product.

Let's try to get that all going by next week.

Justin Schuhmann

On Oct 30, 2015, at 6:12 PM, Spencer Owen notifications@github.com wrote:

@EasyAsABC123 Any update when 4.1.2 or 4.2.0 will be released? I'm at the point where I'm ready for the clear command.


Reply to this email directly or view it on GitHub.

@EasyAsABC123
Copy link
Contributor

@spuder v4.1.3 is released according to GitHub, however it will be a while for chef to merge this code to supermarket as an official release. v4.2.0 will have 2 more features and hopefully some more test-kitchen

@spuder
Copy link
Contributor Author

spuder commented Nov 1, 2015

Two questions

  1. 4.1.3 doesn't appear to have made it into master yet. Is that because of the failing tests?
  2. Should this be refactored to be less 'functional' but to pass the lint tests?
2.42s$ /opt/chefdk/embedded/bin/foodcritic . --exclude spec
FC017: LWRP does not notify when updated: ./providers/config.rb:28
FC017: LWRP does not notify when updated: ./providers/config.rb:32
FC017: LWRP does not notify when updated: ./providers/config.rb:36

@EasyAsABC123
Copy link
Contributor

@spuder

  1. https://github.com/chef-cookbooks/iis/blob/master/providers/config.rb I think it is there
  2. well to make it more complaint we could move the new_resource.updated_by_last_action(true) to be called in every action of the provider.

IE:

# :config deprecated, use :set instead
action :config do
  config
  new_resource.updated_by_last_action(true)
end

action :set do
  config
  new_resource.updated_by_last_action(true)
end

action :clear do
  config(:clear)
  new_resource.updated_by_last_action(true)
end

def config(action = :set)
  cmd = "#{appcmd(node)} #{action} config #{new_resource.cfg_cmd}"
  Chef::Log.debug(cmd)
  shell_out!(cmd, returns: new_resource.returns)
  Chef::Log.info('IIS Config command run')
end

however since shell_out! will return with errors and perhaps not of updated anything at all...i'm going to say no...we'll leave it the way it is

@spuder
Copy link
Contributor Author

spuder commented Nov 2, 2015

Thanks

Looks like master shows 4.1.3 now, (it was showing 4.1.1 before)

354c894#diff-9fd042d4cd1e8f82894bbd7dffb313ec

@EasyAsABC123
Copy link
Contributor

@spuder
gotcha, yeah I don't currently have access to the supermarket. So although the code might be updated I cannot update the supermarket to be correct.

Chef and I are in discussions about this.

For the time being if I need to bring in new changes or bug fixes they might have to go into a different branch than master.

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.

5 participants