Add config command to mange configuration file

[boaz0]

Description

Add config command to kn CLI to read, edit or initialize knative configuration files

Changes

• Add config get
• Add config set (plugins.directory)
• Add config show
• Add config sink-mapping (list / update / add / delete)
• Add config channel-type-mapping (list / update / add / delete)

Reference

closes #1293

/lint

[knative-prow-robot]

Hi @boaz0. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: boaz0 To complete the pull request process, please assign rhuss after the PR has been reviewed. You can assign the PR to them by writing /assign @rhuss in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

[codecov[bot]]

Codecov Report

Base: 79.69% // Head: 79.75% // Increases project coverage by +0.06% :tada:

Coverage data is based on head (f4569b3) compared to base (b696546). Patch coverage: 88.67% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1556      +/-   ##
==========================================
+ Coverage   79.69%   79.75%   +0.06%     
==========================================
  Files         173      179       +6     
  Lines       13264    13363      +99     
==========================================
+ Hits        10571    10658      +87     
- Misses       1963     1971       +8     
- Partials      730      734       +4     

Impacted Files	Coverage Δ
pkg/kn/commands/config/show.go	78.57% <78.57%> (ø)
pkg/kn/commands/config/set.go	81.81% <81.81%> (ø)
pkg/kn/commands/config/get.go	83.33% <83.33%> (ø)
pkg/kn/commands/config/channel_type_mapping.go	100.00% <100.00%> (ø)
pkg/kn/commands/config/config.go	100.00% <100.00%> (ø)
pkg/kn/commands/config/sink_mapping.go	100.00% <100.00%> (ø)
pkg/kn/config/config.go	58.50% <100.00%> (ø)
pkg/kn/root/root.go	89.74% <100.00%> (+0.08%)	:arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[vyasgun]

/ok-to-test

[knative-metrics-robot]

The following is the coverage report on the affected files. Say /test pull-knative-client-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/kn/config/manager.go	Do not exist	0.0%

[knative-prow-robot]

@boaz0: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-knative-client-go-coverage	43c51a70dbff853331badbcf7771dcdafcacca20	link	false	/test pull-knative-client-go-coverage
unit-tests_client_main	7bc7666b84dcade1d13694e1e1aa67a5acbde179	link	true	/test unit-tests_client_main
build-tests_client_main	7bc7666b84dcade1d13694e1e1aa67a5acbde179	link	true	/test build-tests_client_main

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[linux-foundation-easycla[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: boaz0 (7ab82686ecce6ef00706205c07964465c5c16bd2, eef7a423c33580520fb5c617a1e4558dcb9b40a2, 8c93a8739aa408edfee3be6d1641b87a16b6ae94)

[boaz0]

thanks @maximilien your feedback was super helpful. I fixed some of your comments. Feel free to review them.

[dsimansk]

I wonder if a bit unified UI for mappings wouldn't be nicer to group mappings together E.g.:

kn config mapping add <alias> --type {channel,sink} --gvr group:v1:KindOfResource

kn config mapping add kafka --type channel --gvr knative.messaging/v1alpha1:KafkaChannel

The --gvr flag is just another option to discuss. The separate flags are more inline with kn philosophy. However, it's pretty in-depth configuration to deal with.

In addition mapping create would be more consistent with rest of commands, even though technically the command is more on add side than create side.

[knative-prow[bot]]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: boaz0 To complete the pull request process, please ask for approval from dsimansk after the PR has been reviewed.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

[boaz0]

@dsimansk added unit tests. I will now work on the refactoring and move the crud functions to their own directory and change the UI as you suggested above. Hopefully tomorrow I will update the PR with these changes. Thanks.

[boaz0]

/test integration-tests_client_main

[dsimansk]

@boaz0 I've added two comments around cmd naming of show/describe and add/create. I'd like to get @rhuss to chime in as well, but he won't be able around until next week. If you please don't mind to wait a bit for more feedback.

[vyasgun]

Thanks for addressing the comments!

The build is failing due to missing return statements in commands. While addressing that, can you also add examples to the commands like so?: https://github.com/knative/client/blob/main/pkg/kn/commands/broker/create.go#L47

Also, added another comment for using Resource Printer for the list sub-commands by adding ListPrintFlags to the them. For reference: https://github.com/knative/client/blob/main/pkg/kn/commands/broker/list.go#L42

[knative-prow[bot]]

@boaz0: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
build-tests_client_main	f4569b3ec6bc5984595b15e4ce194691c08ab020	link	true	/test build-tests_client_main
integration-tests-latest-release_client_main	f4569b3ec6bc5984595b15e4ce194691c08ab020	link	true	/test integration-tests-latest-release_client_main

Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[boaz0]

@vyasgun - I am not sure exactly what to do with ListPrintFlags. Should I wrap it in Unstructured? The link you gave is not really helping as I am trying to understand what interface BrokerList is implementing and there is a lot of magic (IMHO) so if you can provide a better example or give some useful details I will be grateful.

Thanks.

[vyasgun]

@boaz0 I hope the following explanation and the eventtype command PR (specifically this commit: https://github.com/knative/client/pull/1598/commits/4077ab22a4195e595d6c7dc2f3cde95284303e8e) is helpful. Please let me know if unclear.

The printer pkg is being used for printing. There are two types: machine readable (or generic) and human readable printer. The human readable printer is used by default and can be overridden with the -o flag. To register the handler and add the output related flags to the command, the struct ListPrintFlags is used which is defined as follows:

type ListPrintFlags struct {
	GenericPrintFlags  *genericclioptions.PrintFlags
	HumanReadableFlags *commands.HumanPrintFlags
	PrinterHandler     func(h hprinters.PrintHandler)
}

The PrinterHandler function here is responsible for registering the human readable printers, that is, map the appropriate printer functions to their respective types. To elaborate, take a look at the ListHandler function for eventtype list:

The ListHandler function defines the column definitions and then calls the function TableHandler which registers the printer function + column definitions for the type of the object we are printing (channel-type-mapping and sink-mapping in the current PR's case). Here's the code for this.

To summarise, kn eventtype list will trigger a call to the ListPrintFlags' .Print method. This kickstarts the process of calling the PrintHandler (the one we populated in the ListPrintFlags object) which registers the printer functions and then calls the appropriate printer based on the type of object. (EventtypeList in the example)

[boaz0]

@vyasgun I did follow what you were saying but I still need to implement runtime.Object. I am not sure what DeepCopyObject and GetObjectKind do in my case. Is there a wrapper that does it for me or a way to avoid this? Thanks.

[dsimansk]

@boaz0 indeed those printers are primarily meant to be used with k8s resource objects. IMO for the sake of moving with this PR we can go with your initial impl just printing a plain config.

@rhuss I'd still wonder if you could review the commands naming & UI please.

[rhuss]

Sorry for the delay, I plan to review the PR next week.

[boaz0]

ping @rhuss any updates?

[boaz0]

ping @rhuss @dsimansk any updates? :smiley:

[knative-prow-robot]

@boaz0: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[dsimansk]

@boaz0 hi there, sorry for super late response.

Dialing back on the listing options, since that would require to refactor our Config as a Kubernetes resource.

I'd like to see at least renaming of show command to describe. https://github.com/knative/client/pull/1556#discussion_r896667207. To get this PR rolling again.

Wrt other UI suggestions, I'd like to reconsider the following proposal: https://github.com/knative/client/pull/1556#issuecomment-1152238773

/cc @rhuss

[github-actions[bot]]

This Pull Request is stale because it has been open for 90 days with no activity. It will automatically close after 30 more days of inactivity. Reopen with /reopen. Mark as fresh by adding the comment /remove-lifecycle stale.