Integrated knife-opc into Chef #10187

snehaldwivedi · 2020-07-22T09:18:44Z

Signed-off-by: snehaldwivedi sdwivedi@msystechnologies.com

Description

Merge this knife plugin into Chef.
Moved user password command to core chef

Related Issue

chef-boneyard/knife-opc#48
chef-boneyard/knife-opc#57
#3517
#3514 (covers knife user create should check file permissions), #3515, #3516

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Chore (non-breaking change that does not add functionality or fix an issue)

Checklist:

I have read the CONTRIBUTING document.
I have run the pre-merge tests locally and they pass.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.
All commits have been signed-off for the Developer Certificate of Origin.

stevendanna

Thanks for getting this started!

stevendanna · 2020-07-22T11:23:21Z

lib/chef/knife/opc_org_create.rb

+module Opc
+  class OpcOrgCreate < Chef::Knife
+    category "CHEF ORGANIZATION MANAGEMENT"
+    banner "knife opc org create ORG_SHORT_NAME ORG_FULL_NAME (options)"


We should consider removing the opc prefix from all of these commands.

stevendanna · 2020-07-22T11:37:41Z

lib/chef/knife/opc_org_list.rb

+    def run
+      results = root_rest.get("organizations")
+      unless config[:all_orgs]
+        results = results.select { |k, v| !(k.length == 20 && k =~ /^[a-z]+$/) }


I think this filter is from when we used to pre-generate organizations because organization took a long time. I believe this is no longer the default so perhaps it would be best to turn this filtering off by default and it can end up filtering out legit organizations.

stevendanna · 2020-07-22T11:40:28Z

lib/chef/org/group_operations.rb

+
+class Chef
+  class Org
+    module GroupOperations


I wonder if we really need a separate module for this. It was structured this way in the plugin because we lived outside the Chef codebase, but we could probably add these features directly to the Chef::Org class or create a new Chef::Group class that would abstract away some of these features.

stevendanna · 2020-07-22T11:41:54Z

lib/chef/knife/opc_user_create.rb

+module Opc
+  class OpcUserCreate < Chef::Knife
+    category "CHEF ORGANIZATION MANAGEMENT"
+    banner "knife opc user create USERNAME FIRST_NAME [MIDDLE_NAME] LAST_NAME EMAIL PASSWORD"


We already have a set of knife user commands that I think should cover most of the user-related commands in this PR.

I'd like to see one or the other, but not both user commands. Since every chef server is a server with an org at this point we should just make any missing functionality part of the existing knife user operations.

stevendanna · 2020-07-22T11:46:39Z

lib/chef/mixin/root_rest.rb

+        # Rather than upgrade all of this code to move to v1, the goal is to remove the
+        # need for this plugin.  See
+        # https://github.com/chef/chef/issues/3517
+        @root_rest ||= Chef::ServerAPI.new(Chef::Config[:chef_server_root], { api_version: "0" })


I believe that the existing user plugins were converted to v1 and that none of the other APIs here really care about the API version so we can probably don't need to specify the api_version if we end up removing the user-related commands from this PR.

Yeah this should go away and we should not be using v0 here.

snehaldwivedi · 2020-07-22T11:55:18Z

NOTE: This test is failing due to changes in gem chef/.bundle/ruby/2.7.0/gems/tty-prompt-0.21.0/lib/tty/prompt/question.rb
attr_accessor :echo need to be added here

snehaldwivedi · 2020-07-29T05:44:04Z

I am working on all review comments also I found that both knife user and knife opc user commands don't match completely, like for example knife opc user list also provide list of user associated with organisations which is not available in knife user list command. So I think we can merge that too and started working on this as well.

lamont-granquist · 2020-08-03T17:38:29Z

lib/chef/knife/org_delete.rb

+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+require_relative "../mixin/root_rest"


this mixin still needs to go away

tas50 · 2020-08-03T17:38:58Z

lib/chef/knife/user_list.rb

@@ -1,6 +1,6 @@
 #
 # Author:: Steven Danna (<steve@chef.io>)
-# Copyright:: Copyright (c) Chef Software Inc.
+# Copyright:: Copyright 2011-2016 Chef Software, Inc.


None of these copyrights need to get updated

kvivek1115 · 2020-08-07T03:39:19Z

lib/chef/knife/user_create.rb

          end
        end
      end
+
+      def prompt_for_password
+        ui.ask("Please enter the user's password: ") { |q| q.echo = false }


Suggested change

ui.ask("Please enter the user's password: ") { |q| q.echo = false }

ui.ask("Please enter the user's password: ", echo: false)

btm · 2020-08-07T13:11:49Z

@snehaldwivedi if knife org edit survives into core, please test it to see that it opens the editor? chef-boneyard/knife-opc#57

lamont-granquist · 2020-08-10T17:10:20Z

lib/chef/knife/org_show.rb


      def run
        org_name = @name_args[0]
-        ui.output root_rest.get("organizations/#{org_name}")
+        org = Chef::Org.from_hash({ "name" => org_name })
+        ui.output org.chef_rest.get("organizations/#{org_name}")


This seems like this should be pushed into the Chef::Org object.

I haven't looked but we should be able to do something to get a Chef::Org object for the org_name directly rather than going in through from_hash and then the get call should be hidden from the caller. That could be combined into one static method if necessary like Chef::Org.load(org_name) (unless that collides with an existing method name but something like that)

okay I will look into that

@lamont-granquist I would prefer to add root_rest method to knife.rb similar to rest so that it will be available to all subclasses as we have more root level operations into chef core.

def root_rest @root_rest ||= begin require_relative "server_api" Chef::ServerAPI.new(Chef::Config[:chef_server_root]) end end

So the point here though is that AFAIK we should not be using protocol v0, and I don't understand why this PR is spreading it everywhere.

The chef/user.rb that defines the chef_rest_v0 object has a large banner at the top of it that Chef::User should be replaced by Chef::UserV1 in Chef 13 (that didn't happen) but AFAIK it should all go away. I have no idea why knife-opc was using v0, that may have been due to some kind of backwards compatibility with now-ancient chef-servers. Without knowing what breaks, my position is that it should be removed and we should be using our normal v2 rest objects consistently.

knife-opc was using v0 simply because we needed it to work ASAP and that was the smallest possible change, so 👍 to doing it right this time around.

tas50 · 2020-08-21T04:29:29Z

lib/chef/knife/user_delete.rb

+      option :no_disassociate_user,
+        long: "--no-disassociate-user",
+        short: "-d",
+        description: "Don't disassociate the user first"


disassociate from what? This description isn't very helpful

I will update it

for disassociate user I will update once this is merge chef-boneyard/knife-opc#36 as it has changes related to it

@tas50 can you please confirm if chef-boneyard/knife-opc#56 is good to add changes in this PR?

snehaldwivedi · 2020-10-09T11:10:17Z

I have removed abstract_user as we are not covering cleanup #10458 on this PR. Updated API version back to 1 and used user_v1 for commands that require any helper methods else used directly rest objects.

Please let me know if require any changes

snehaldwivedi · 2020-10-30T08:22:17Z

@tas50 Can you please have a look into the PR for merge or if any more changes required form my side?

lamont-granquist · 2021-02-01T18:30:33Z

You should be able to eliminate the @root_rest API object entirely and just use the default knife rest object. That is a v2 REST object but it should work fine with the users_v1 object. They don't have to match the rest object just needs to be a higher version, which it should be. It would break with users_v0 which is deprecated at this point.

snehaldwivedi · 2021-02-04T09:11:31Z

Hi @lamont-granquist

As per my analysis, I cannot use the default rest object as its uses chef_server_url which will not provide us the exact result required for the opc commands. Because chef_server_url is URL for the specific organizations which will not provide the proper result like if we need a list of all organization, global user requirement in this ticket.

If we can change chef_server_url to chef_server_root in rest object then we can use it. Please let me know if you have any suggestions on this or any other way to do this?

lamont-granquist · 2021-02-10T00:53:29Z

Okay the communication failure is now a bit more obvious. You can do that, but you need to remove { api_version: "1" } off of the arguments, that's the problem that I'm having with it.

lamont-granquist · 2021-02-15T18:37:10Z

lib/chef/knife.rb

@@ -640,7 +640,7 @@ def test_mandatory_field(field, fieldname)
    def rest
      @rest ||= begin
        require_relative "server_api"
-        Chef::ServerAPI.new(Chef::Config[:chef_server_url])
+        Chef::ServerAPI.new(Chef::Config[:chef_server_root])


doesn't this change the rest API for all the rest of the knife commands though?

i was okay with the root_rest object but just didn't want it v1.

Signed-off-by: snehaldwivedi <sdwivedi@msystechnologies.com>

This reverts commit 182d836.

Signed-off-by: snehaldwivedi <sdwivedi@msystechnologies.com>

snehaldwivedi requested review from a team as code owners July 22, 2020 09:18

snehaldwivedi changed the title ~~Merge this knife plugin into Chef directly~~ Integrated knife-opc into Chef Jul 22, 2020

stevendanna reviewed Jul 22, 2020

View reviewed changes

tas50 added the Status: Waiting on Contributor A pull request that has unresolved requested actions from the author. label Jul 27, 2020

snehaldwivedi force-pushed the snehal/Merge_knife_plugin_into_Chef branch from d8deb15 to 76ffdf5 Compare July 31, 2020 11:47

lamont-granquist suggested changes Aug 3, 2020

View reviewed changes

tas50 reviewed Aug 3, 2020

View reviewed changes

snehaldwivedi mentioned this pull request Aug 4, 2020

knife osc user list and knife opc user list give different results chef-boneyard/knife-opc#41

Closed

snehaldwivedi force-pushed the snehal/Merge_knife_plugin_into_Chef branch from 7956edd to 6146928 Compare August 6, 2020 13:38

kvivek1115 reviewed Aug 7, 2020

View reviewed changes

snehaldwivedi force-pushed the snehal/Merge_knife_plugin_into_Chef branch from 6146928 to 30cde90 Compare August 7, 2020 05:01

snehaldwivedi force-pushed the snehal/Merge_knife_plugin_into_Chef branch from c9d41dc to 3842097 Compare August 10, 2020 13:43

lamont-granquist reviewed Aug 10, 2020

View reviewed changes

btm linked an issue Aug 13, 2020 that may be closed by this pull request

Merge knife-opc into core Chef #3517

Closed

8 tasks

btm assigned snehaldwivedi Aug 13, 2020

tas50 reviewed Aug 21, 2020

View reviewed changes

snehaldwivedi force-pushed the snehal/Merge_knife_plugin_into_Chef branch 4 times, most recently from 4a154f9 to e419e96 Compare September 2, 2020 09:25

snehaldwivedi mentioned this pull request Sep 2, 2020

Update all knife client commands with API version 2 #10373

Closed

snehaldwivedi force-pushed the snehal/Merge_knife_plugin_into_Chef branch from e419e96 to 38d874a Compare September 3, 2020 05:50

This was referenced Sep 3, 2020

knife client create should check file permissions #10380

Closed

knife user show should have an option to include org membership #3515

Closed

No knife org subcommands exist #3516

Closed

snehaldwivedi force-pushed the snehal/Merge_knife_plugin_into_Chef branch 2 times, most recently from 3a7e23b to 182d836 Compare February 11, 2021 12:27

lamont-granquist reviewed Feb 15, 2021

View reviewed changes

snehaldwivedi added 14 commits February 16, 2021 02:45

Merge this knife plugin into Chef directly

1f655be

Signed-off-by: snehaldwivedi <sdwivedi@msystechnologies.com>

Updated reviewed changes

f9f3fe3

Signed-off-by: snehaldwivedi <sdwivedi@msystechnologies.com>

Fix Typo

43c05d7

Signed-off-by: snehaldwivedi <sdwivedi@msystechnologies.com>

Updated reviewed changes for copyrights and roo_rest

c9080be

Signed-off-by: snehaldwivedi <sdwivedi@msystechnologies.com>

Updated reviewed changes

37ac3c3

Signed-off-by: snehaldwivedi <sdwivedi@msystechnologies.com>

Fixed changes for prompt password

7ead343

Signed-off-by: snehaldwivedi <sdwivedi@msystechnologies.com>

knife org edit NAME does not work

16d0f32

Signed-off-by: snehaldwivedi <sdwivedi@msystechnologies.com>

Updated changes to remove userV1 and use root_rest object

ad08878

Signed-off-by: snehaldwivedi <sdwivedi@msystechnologies.com>

Updated changes for variable name and class name

b64b107

Signed-off-by: snehaldwivedi <sdwivedi@msystechnologies.com>

Remove unused file

ee81245

Signed-off-by: snehaldwivedi <sdwivedi@msystechnologies.com>

Updated API verion back to 1 and used user_v1

3c1bf66

Signed-off-by: snehaldwivedi <sdwivedi@msystechnologies.com>

Updated rest object URL and removed api_version 1

8f32723

Signed-off-by: snehaldwivedi <sdwivedi@msystechnologies.com>

Revert "Updated rest object URL and removed api_version 1"

6a35878

This reverts commit 182d836.

To remove verison v1 for root_rest object

0570757

Signed-off-by: snehaldwivedi <sdwivedi@msystechnologies.com>

snehaldwivedi force-pushed the snehal/Merge_knife_plugin_into_Chef branch from 182d836 to 0570757 Compare February 16, 2021 10:45

lamont-granquist approved these changes Feb 22, 2021

View reviewed changes

lamont-granquist merged commit 2e7c4c7 into master Feb 22, 2021

lamont-granquist deleted the snehal/Merge_knife_plugin_into_Chef branch February 22, 2021 18:33

This was referenced Feb 22, 2021

Improve user delete and handle errors chef-boneyard/knife-opc#56

Closed

Cleanup of bugs and better error handling in user delete. chef-boneyard/knife-opc#36

Closed

snehaldwivedi mentioned this pull request Feb 23, 2021

Add global list option in knife user list #10456

Closed

10 tasks

snehaldwivedi mentioned this pull request Mar 25, 2021

knife client create should check file permissions #11241

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrated knife-opc into Chef #10187

Integrated knife-opc into Chef #10187

snehaldwivedi commented Jul 22, 2020 •

edited

Loading

stevendanna left a comment

stevendanna Jul 22, 2020

tas50 Jul 22, 2020

stevendanna Jul 22, 2020

stevendanna Jul 22, 2020

stevendanna Jul 22, 2020

tas50 Jul 22, 2020

stevendanna Jul 22, 2020

lamont-granquist Jul 27, 2020

snehaldwivedi commented Jul 22, 2020 •

edited

Loading

snehaldwivedi commented Jul 29, 2020 •

edited

Loading

lamont-granquist Aug 3, 2020

tas50 Aug 3, 2020

kvivek1115 Aug 7, 2020

btm commented Aug 7, 2020

lamont-granquist Aug 10, 2020

snehaldwivedi Aug 11, 2020

kvivek1115 Aug 11, 2020

lamont-granquist Aug 12, 2020

stevendanna Aug 24, 2020

tas50 Aug 21, 2020

snehaldwivedi Aug 24, 2020

snehaldwivedi Sep 2, 2020

snehaldwivedi Oct 30, 2020

snehaldwivedi commented Oct 9, 2020 •

edited

Loading

snehaldwivedi commented Oct 30, 2020

lamont-granquist commented Feb 1, 2021

snehaldwivedi commented Feb 4, 2021 •

edited

Loading

lamont-granquist commented Feb 10, 2021

lamont-granquist Feb 15, 2021

	ui.ask("Please enter the user's password: ") { \|q\| q.echo = false }
	ui.ask("Please enter the user's password: ", echo: false)

Integrated knife-opc into Chef #10187

Integrated knife-opc into Chef #10187

Conversation

snehaldwivedi commented Jul 22, 2020 • edited Loading

Description

Related Issue

Types of changes

Checklist:

stevendanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snehaldwivedi commented Jul 22, 2020 • edited Loading

snehaldwivedi commented Jul 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

btm commented Aug 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snehaldwivedi commented Oct 9, 2020 • edited Loading

snehaldwivedi commented Oct 30, 2020

lamont-granquist commented Feb 1, 2021

snehaldwivedi commented Feb 4, 2021 • edited Loading

lamont-granquist commented Feb 10, 2021

Choose a reason for hiding this comment

snehaldwivedi commented Jul 22, 2020 •

edited

Loading

snehaldwivedi commented Jul 22, 2020 •

edited

Loading

snehaldwivedi commented Jul 29, 2020 •

edited

Loading

snehaldwivedi commented Oct 9, 2020 •

edited

Loading

snehaldwivedi commented Feb 4, 2021 •

edited

Loading