Added autocompletion for service name #1547

vyasgun · 2021-12-14T04:45:59Z

Description

Added ValidArgsFunc for autocompletion during kn service describe and kn service update

Changes

Added ValidArgsFunc to service describe and service update commands

./kn service update hello-example<tab>
hello-example    hello-example-2  hello-example-3  hello-example-4

Reference

Fixes #1373

knative-prow-robot

@vyasgun: 2 warnings.

In response to this:

Description

Added ValidArgsFunc for autocompletion during kn service describe and kn service update

Changes

Added ValidArgsFunc to service describe and service update commands
./kn service update hello-example<tab>
hello-example    hello-example-2  hello-example-3  hello-example-4
Reference

Fixes #1373

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.

pkg/kn/commands/completion_helper.go

codecov · 2021-12-14T04:49:59Z

Codecov Report

Merging #1547 (5feffed) into main (972eec2) will increase coverage by 0.01%.
The diff coverage is 93.65%.

@@            Coverage Diff             @@
##             main    #1547      +/-   ##
==========================================
+ Coverage   79.56%   79.57%   +0.01%     
==========================================
  Files         162      163       +1     
  Lines        8553     8544       -9     
==========================================
- Hits         6805     6799       -6     
+ Misses       1071     1068       -3     
  Partials      677      677

Impacted Files	Coverage Δ
pkg/kn/commands/service/delete.go	`80.32% <ø> (ø)`
pkg/kn/commands/completion_helper.go	`92.15% <92.15%> (ø)`
pkg/kn/commands/namespaced.go	`92.50% <100.00%> (+0.60%)`	⬆️
pkg/kn/commands/service/describe.go	`73.62% <100.00%> (+0.14%)`	⬆️
pkg/kn/commands/service/update.go	`81.69% <100.00%> (+0.26%)`	⬆️
cmd/kn/main.go	`66.66% <0.00%> (-20.66%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 972eec2...5feffed. Read the comment docs.

rhuss · 2021-12-21T11:53:06Z

Thanks a ton, @vyasgun ! I hope I can have a look still before the break and lets get it merged then. Great stuff :)

vyasgun · 2021-12-21T12:34:49Z

@rhuss Thanks! It would be great if you could take a look. I can implement for other resources similarly

rhuss

Thanks ! (also for adding the extra tests ;-)

Looks good to me on a first view, I have some minor remarks.

rhuss · 2021-12-22T15:21:33Z

pkg/kn/commands/completion_helper.go

+
+// ResourceNameCompletionFunc will return a function that will autocomplete the name of
+// the resource based on the subcommand
+func ResourceNameCompletionFunc(p *KnParams) func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {


Is there a case when not cobra.ShellCompDirectiveNoFileComp is returned ? Because then you could remove that return value.

cobra.ShellCompDirectiveNoFileComp is being returned in all cases. The function being returned needs to be of this specific signature to be used as a ValidArgsFunction.

rhuss · 2021-12-22T15:25:31Z

pkg/kn/commands/completion_helper.go

+	command    *cobra.Command
+	args       []string
+	toComplete string
+	target     string


I wonder whether target needs to be in an extra field. It's specific only to service commands and could easily be extracted from the command when it is already clear that we are dealing with services.

yes, I think we can remove the field

rhuss · 2021-12-22T15:29:17Z

pkg/kn/commands/namespaced.go

@@ -43,7 +45,11 @@ func AddNamespaceFlags(flags *pflag.FlagSet, allowAll bool) {

 // GetNamespace returns namespace from command specified by flag
 func (params *KnParams) GetNamespace(cmd *cobra.Command) (string, error) {
-	namespace := cmd.Flag("namespace").Value.String()
+	namespaceFlag := cmd.Flag("namespace")
+	if namespaceFlag == nil {


thanks for adding this check !

thank you :)

knative-metrics-robot · 2022-01-04T10:30:35Z

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/commands/completion_helper.go	Do not exist	96.0%
pkg/kn/commands/namespaced.go	96.3%	96.7%	0.4

dsimansk

Thanks!

/approve
/lgtm

knative-prow-robot · 2022-01-04T12:31:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dsimansk, vyasgun

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [dsimansk]

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

knative-prow-robot reviewed Dec 14, 2021

View reviewed changes

pkg/kn/commands/completion_helper.go Show resolved Hide resolved

pkg/kn/commands/completion_helper.go Outdated Show resolved Hide resolved

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 14, 2021

vyasgun force-pushed the pr/completion branch 2 times, most recently from 088b3c2 to c58eb26 Compare December 21, 2021 09:27

vyasgun added 3 commits December 22, 2021 20:27

Added autocompletion for service name

d799e22

Added unit tests

745218b

Added error handling for panic if no namespace flag added to command

eea74d1

vyasgun force-pushed the pr/completion branch from c58eb26 to eea74d1 Compare December 22, 2021 15:00

rhuss reviewed Dec 22, 2021

View reviewed changes

vyasgun added 2 commits December 22, 2021 21:50

Removed target field from config

7ca9088

Modified signature for completion functions

5feffed

dsimansk approved these changes Jan 4, 2022

View reviewed changes

knative-prow-robot assigned dsimansk Jan 4, 2022

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 4, 2022

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 4, 2022

knative-prow-robot merged commit 6314298 into knative:main Jan 4, 2022

rhuss mentioned this pull request Jan 5, 2022

Context sensitive Bash completion #1373

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added autocompletion for service name #1547

Added autocompletion for service name #1547

vyasgun commented Dec 14, 2021

knative-prow-robot left a comment

codecov bot commented Dec 14, 2021 •

edited

Loading

rhuss commented Dec 21, 2021

vyasgun commented Dec 21, 2021

rhuss left a comment

rhuss Dec 22, 2021

vyasgun Dec 22, 2021 •

edited

Loading

rhuss Dec 22, 2021

vyasgun Dec 22, 2021

rhuss Dec 22, 2021

vyasgun Dec 22, 2021

knative-metrics-robot commented Jan 4, 2022

dsimansk left a comment

knative-prow-robot commented Jan 4, 2022

Added autocompletion for service name #1547

Added autocompletion for service name #1547

Conversation

vyasgun commented Dec 14, 2021

Description

Changes

Reference

knative-prow-robot left a comment

Choose a reason for hiding this comment

Description

Changes

Reference

codecov bot commented Dec 14, 2021 • edited Loading

Codecov Report

rhuss commented Dec 21, 2021

vyasgun commented Dec 21, 2021

rhuss left a comment

Choose a reason for hiding this comment

rhuss Dec 22, 2021

Choose a reason for hiding this comment

vyasgun Dec 22, 2021 • edited Loading

Choose a reason for hiding this comment

rhuss Dec 22, 2021

Choose a reason for hiding this comment

vyasgun Dec 22, 2021

Choose a reason for hiding this comment

rhuss Dec 22, 2021

Choose a reason for hiding this comment

vyasgun Dec 22, 2021

Choose a reason for hiding this comment

knative-metrics-robot commented Jan 4, 2022

dsimansk left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Jan 4, 2022

codecov bot commented Dec 14, 2021 •

edited

Loading

vyasgun Dec 22, 2021 •

edited

Loading