Add completion of resource names for args and flags #102

ericbottard · 2019-10-21T10:41:49Z

Fixes #74

ericbottard · 2019-10-21T10:44:17Z

pkg/riff/commands/custom_completion.go

+    local template
+    template="{{ range .items  }}{{ .metadata.name }} {{ end }}"
+    local kubectl_out
+    if kubectl_out=$(kubectl get -o template --template="${template}" --selector streaming.projectriff.io/provisioner services 2>/dev/null); then


TODO in server

what do you mean by "in server"?

codecov · 2019-10-21T13:00:39Z

Codecov Report

Merging #102 into master will increase coverage by 0.13%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #102      +/-   ##
==========================================
+ Coverage   95.71%   95.85%   +0.13%     
==========================================
  Files          86       86              
  Lines        4897     5061     +164     
==========================================
+ Hits         4687     4851     +164     
  Misses        186      186              
  Partials       24       24

Impacted Files	Coverage Δ
pkg/build/commands/credential_apply.go	`98.41% <100%> (ø)`	⬆️
pkg/streaming/commands/processor_create.go	`100% <100%> (ø)`	⬆️
pkg/knative/commands/adapter_create.go	`99.37% <100%> (+0.02%)`	⬆️
pkg/riff/commands/completion.go	`99.45% <100%> (+2.09%)`	⬆️
pkg/cli/flags.go	`100% <100%> (ø)`	⬆️
pkg/build/commands/application_create.go	`96.98% <100%> (+0.01%)`	⬆️
pkg/core/commands/deployer_create.go	`98.36% <100%> (+0.02%)`	⬆️
pkg/build/commands/function_create.go	`96.92% <100%> (+0.01%)`	⬆️
pkg/riff/commands/root.go	`95.71% <100%> (+0.19%)`	⬆️
pkg/streaming/commands/stream_create.go	`100% <100%> (ø)`	⬆️

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 d7ad872...da1bad0. Read the comment docs.

scothis

What's here seems to work well in the happy case, but there are a number of unaccounted for corner cases

scothis · 2019-10-21T13:56:36Z

pkg/streaming/commands/processor_create.go

@@ -153,6 +153,8 @@ func NewProcessorCreateCommand(ctx context.Context, c *cli.Config) *cobra.Comman

 	cli.NamespaceFlag(cmd, c, &opts.Namespace)
 	cmd.Flags().StringVar(&opts.FunctionRef, cli.StripDash(cli.FunctionRefFlagName), "", "`name` of function build to deploy")
+	_ = cmd.MarkFlagRequired(cli.StripDash(cli.FunctionRefFlagName))


rm. We handle required fields in the validator

scothis · 2019-10-21T13:56:49Z

pkg/streaming/commands/stream_create.go

@@ -110,6 +110,8 @@ func NewStreamCreateCommand(ctx context.Context, c *cli.Config) *cobra.Command {

 	cli.NamespaceFlag(cmd, c, &opts.Namespace)
 	cmd.Flags().StringVar(&opts.Provider, cli.StripDash(cli.ProviderFlagName), "", "`name` of stream provider")
+	_ = cmd.MarkFlagRequired(cli.StripDash(cli.ProviderFlagName))


rm. We handle required fields in the validator

scothis · 2019-10-21T13:57:33Z

pkg/streaming/commands/kafkaprovider_create.go

@@ -102,6 +102,7 @@ func NewKafkaProviderCreateCommand(ctx context.Context, c *cli.Config) *cobra.Co

 	cli.NamespaceFlag(cmd, c, &opts.Namespace)
 	cmd.Flags().StringVar(&opts.BootstrapServers, cli.StripDash(cli.BootstrapServersFlagName), "", "`address` of the kafka broker")
+	_ = cmd.MarkFlagRequired(cli.StripDash(cli.BootstrapServersFlagName))


rm. We handle required fields in the validator

scothis · 2019-10-21T13:58:48Z

pkg/riff/commands/root.go

@@ -36,6 +36,7 @@ func NewRootCommand(ctx context.Context, c *cli.Config) *cobra.Command {

 	cmd.Use = c.Name
 	cmd.DisableAutoGenTag = true
+	cmd.BashCompletionFunction = bash_completion_func


Seems like this should be defined in by completion command. If we can't set the value in a PreRun, then pass the root command to NewCompletionCommand.

Going a step further. It might be nice to define the specific completion bits closer to each command group/runtime. And use a command walker to assemble all of the particular bits.

👍
The completion examples available are not particularly smart. Neither am I when it comes to bash, so...

scothis · 2019-10-21T14:01:46Z

pkg/riff/commands/custom_completion.go

+__kubectl_get_namespaces()
+{
+    local template
+    template="{{ range .items  }}{{ .metadata.name }} {{ end }}"


is the space before {{ end }} intentional?

Suggested change

template="{{ range .items }}{{ .metadata.name }} {{ end }}"

template="{{ range .items }}{{ .metadata.name }} {{ end }}"

This is pure copy pasta from the Cobra page, feel free to adjust.

scothis · 2019-10-21T14:07:56Z

pkg/riff/commands/custom_completion.go

+__kubectl_get_streaming_provisioner_services()
+{
+    local template
+    template="{{ range .items  }}{{ .metadata.name }} {{ end }}"


Suggested change

template="{{ range .items }}{{ .metadata.name }} {{ end }}"

template="{{ range .items }}{{ .metadata.name }} {{ end }}"

scothis · 2019-10-21T14:08:42Z

pkg/riff/commands/custom_completion.go

+    local template
+    template="{{ range .items  }}{{ .metadata.name }} {{ end }}"
+    local kubectl_out
+    if kubectl_out=$(kubectl get -o template --template="${template}" --selector streaming.projectriff.io/provisioner services 2>/dev/null); then


what do you mean by "in server"?

scothis · 2019-10-21T14:17:45Z

pkg/riff/commands/custom_completion.go

+	__riff_debug "listing $1"
+    local riff_output out
+    if riff_output=$(riff $1 2>/dev/null); then
+        out=($(echo "${riff_output}" | awk 'NR>1 {print $1}'))


this assumes the name will be the first column, which is not always true, but probably true enough

scothis · 2019-10-21T14:22:09Z

pkg/riff/commands/custom_completion.go

+}
+
+__riff_custom_func() {
+    case ${last_command} in


what if someone uses an alias or the cli is built with a different name?

scothis · 2019-10-21T14:33:51Z

pkg/build/commands/application_create.go

@@ -282,6 +282,7 @@ cluster).
 	cmd.Flags().StringVar(&opts.Image, cli.StripDash(cli.ImageFlagName), "_", "`repository` where the built images are pushed")
 	cmd.Flags().StringVar(&opts.CacheSize, cli.StripDash(cli.CacheSizeFlagName), "", "`size` of persistent volume to cache resources between builds")
 	cmd.Flags().StringVar(&opts.LocalPath, cli.StripDash(cli.LocalPathFlagName), "", "path to `directory` containing source code on the local machine")
+	_ = cmd.MarkFlagDirname(cli.StripDash(cli.LocalPathFlagName))


nit, drop the assignment

the convention in this repo is to drop blank identifier assignments unless they are needed

GoLand is pretty aggressive on the eyes for unhandled errors. I like explicit assignments that mention that we know what we're doing (at least at the time of authoring). I'm for 100% consistency though, so...

which linter are you using?

Just vanilla GoLand which highlights constructs like these. It's pretty effective, but also noisy in these cases:

scothis

@ericbottard I addressed my main points of feedback. Back to you for approval/merge.

Refs projectriff/cli#102

ericbottard · 2019-10-22T14:22:40Z

Merged as e92ef2d

Add completion of resource names for args

ecd1f53

Fixes projectriff#74

ericbottard requested a review from scothis October 21, 2019 10:41

ericbottard commented Oct 21, 2019

View reviewed changes

ericbottard added this to the v0.5.0 milestone Oct 21, 2019

Add flag completion

e9356dd

ericbottard force-pushed the issue_74 branch from c46eb9b to e9356dd Compare October 21, 2019 12:57

scothis suggested changes Oct 21, 2019

View reviewed changes

scothis added 6 commits October 21, 2019 12:19

backout MarkFlagRequired

b8970fd

Avoid __kubectl as it's used for kubectl's completion

6fa39e2

pass through --kubeconfig --namespace -n flags

708e68a

Align --kube-config with kubectl's --kubeconfig

32a39f6

Mark --gcr as a file

b38db4a

Decouple from 'riff' command name

108ba48

scothis approved these changes Oct 21, 2019

View reviewed changes

polish

bae5552

scothis added a commit to scothis/system that referenced this pull request Oct 21, 2019

Add common label to provisioner resources

b0a0e38

Refs projectriff/cli#102

This was referenced Oct 21, 2019

Add common label to provisioner resources projectriff/system#145

Merged

CLI DX: kafka provider name does not match --provider option for stream create #101

Closed

ericbottard pushed a commit to projectriff/system that referenced this pull request Oct 22, 2019

Add common label to provisioner resources

b45854a

Refs projectriff/cli#102

remove array conversion for strings, since they are not arrays

da1bad0

ericbottard closed this Oct 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add completion of resource names for args and flags #102

Add completion of resource names for args and flags #102

ericbottard commented Oct 21, 2019

ericbottard Oct 21, 2019

scothis Oct 21, 2019

codecov bot commented Oct 21, 2019 •

edited

Loading

scothis left a comment

scothis Oct 21, 2019

scothis Oct 21, 2019

scothis Oct 21, 2019

scothis Oct 21, 2019

ericbottard Oct 21, 2019

scothis Oct 21, 2019

scothis Oct 21, 2019

ericbottard Oct 21, 2019

scothis Oct 21, 2019

scothis Oct 21, 2019

scothis Oct 21, 2019

scothis Oct 21, 2019

scothis Oct 21, 2019

ericbottard Oct 21, 2019

scothis Oct 21, 2019

ericbottard Oct 21, 2019

scothis left a comment

ericbottard commented Oct 22, 2019

	template="{{ range .items }}{{ .metadata.name }} {{ end }}"
	template="{{ range .items }}{{ .metadata.name }} {{ end }}"

Add completion of resource names for args and flags #102

Add completion of resource names for args and flags #102

Conversation

ericbottard commented Oct 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 21, 2019 • edited Loading

Codecov Report

scothis 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

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

scothis left a comment

Choose a reason for hiding this comment

ericbottard commented Oct 22, 2019

codecov bot commented Oct 21, 2019 •

edited

Loading