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

Fixes #123: Add option to prune deleted clients #124

Closed
wants to merge 4 commits into from

Conversation

@thg65
Copy link

commented Nov 26, 2014

Added an option (--prune-clients) that allows to prune clients that have
been deleted when removing clients from a vault's access list (knife
vault remove [--prune-clients] .... This has been implemented by adding
an optional prune argument to rotate_keys (false by default) which is
set in VaultRemove if --prune-clients has been specified.

Added an option (--prune-clients) that allows to prune clients that have
been deleted when removing clients from a vault's access list (knife
vault remove [--prune-clients] ....  This has been implemented by adding
an optional prune argument to rotate_keys (false by default) which is
set in VaultRemove if --prune-clients has been specified.
@jf647

This comment has been minimized.

Copy link

commented on 7122ef6 Nov 29, 2014

Thanks for the pull request.

In (order to merge this, can you please:

  • add tests for the new functionality (there is now an Aruba framework for testing CLI options which you can build on top of)
  • remove the version bump. This is new functionality, not a bugfix, so when it gets released it would have to be v2.4.0
  • add the same functionality to vault_rotate_all_keys.rb to keep things consistent

Thanks

This comment has been minimized.

Copy link
Owner Author

replied Dec 1, 2014

Hi James,

Added the tests. It took a bit longer since I first needed to realize why the clean tests were failing on my box. I am working inside a VM and it seems the default timeout of 3 seconds for invoking knife commands is too small. The command took 3.2s to correctly execute on my box. :(

Removed the version bump (but kept the bullet in the Changelog (but please feel free to update it to your liking).

This option is now available for
knife vault remove
knife vault rotate keys
knife vault rotate all keys

Let me know if there is anything that can/shall be improved further.

Best,
Thomas

This comment has been minimized.

Copy link
Owner Author

replied Dec 3, 2014

This comment has been minimized.

Copy link

replied Dec 3, 2014

Hi Thomas

I see the changes; thank you.

I'm going to try to wrap this into a 2.4.0 release today. There were some internal comments as to whether --prune-clients was the best wording, so we might change that slightly, but otherwise things look good.

Thanks

This comment has been minimized.

Copy link
Owner Author

replied Dec 3, 2014

thg65 added 2 commits Dec 1, 2014
As requested by James, add the --prune-clients option to rotate (all)
keys commands as well.  Undo the version bump.

Also added documentation of the --prune-clients switch to the documentation.
Added an option (--prune-clients) that allows to prune clients that have
been deleted from Chef when rotating vault keys.  This option is
available for the knife vault remove|rotate all keys|rotate keys
commands.  This has been implemented by adding an optional prune
argument to the rotate_keys and rotate_all_keys functions (false by
default).

Also added aruba CLI tests to test the option in combination with each
of the three commands.
@jf647

This comment has been minimized.

Copy link
Contributor

commented Dec 3, 2014

A quick question on the PR: I see the new step definitions for deleting and removing clients, but I do not see a feature that uses them:

M00972939 ➜  chef-vault git:(thg65-prune-clients-branch) ✗ grep -ri 'i delete' .
./features/step_definitions/chef-repo.rb:When /^I delete clients? '(.+)' from the Chef server$/ do |nodelist|
M00972939 ➜  chef-vault git:(thg65-prune-clients-branch) ✗ grep -ri 'i remove' .
./features/step_definitions/chef-vault.rb:When /^I remove clients? '(.+)' from vault item '(.+)\/(.+)' with the '(.+)' options?$/ do |nodelist, vault, item, optionlist|
M00972939 ➜  chef-vault git:(thg65-prune-clients-branch) ✗

Did those get lost from the commit/PR?

Thanks

@thg65

This comment has been minimized.

Copy link
Author

commented Dec 3, 2014

Hi James,

Yes, of course, forgot to add the file I have added - very sorry. I
committed it - can create the push request tmr.

In case you it is easier/faster as attachment, here it is.
(See attached file: prune.feature)

Best,

Thomas

Thomas Gschwind Email: thg@zurich.ibm.com
IBM Zurich Research Lab
Saeumerstrasse 4 Tel: +41-44-724-8990
CH-8803 Rueschlikon, Switzerland Fax: +41-44-724-8953

James FitzGibbon notifications@github.com wrote on 03/12/2014 21:32:11:

From: James FitzGibbon notifications@github.com
To: Nordstrom/chef-vault chef-vault@noreply.github.com
Cc: Thomas Gschwind thg@zurich.ibm.com
Date: 03/12/2014 21:32
Subject: Re: [chef-vault] Fixes #123: Add option to prune deleted
clients (#124)

A quick question on the PR: I see the new step definitions for
deleting and removing clients, but I do not see a feature that uses them:
M00972939 ➜ chef-vault git:(thg65-prune-clients-branch) ✗ grep -ri
'i delete' .
./features/step_definitions/chef-repo.rb:When /^I delete clients?
'(.+)' from the Chef server$/ do |nodelist|
M00972939 ➜ chef-vault git:(thg65-prune-clients-branch) ✗ grep -ri
'i remove' .
./features/step_definitions/chef-vault.rb:When /^I remove clients?
'(.+)' from vault item '(.+)/(.+)' with the '(.+)' options?$/ do |
nodelist, vault, item, optionlist|
M00972939 ➜ chef-vault git:(thg65-prune-clients-branch) ✗

Did those get lost from the commit/PR?
Thanks

Reply to this email directly or view it on GitHub.[image removed]

@jf647

This comment has been minimized.

Copy link
Contributor

commented Dec 3, 2014

I've integrated this and released it as part of v2.4.0

Thanks for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.