feat(kafka list): add search flag by rkpattnaik780 · Pull Request #364 · redhat-developer/app-services-cli

rkpattnaik780 · 2021-02-19T13:03:48Z

Search flag has been added to kafka list to filter by text.

fixes #181

craicoverflow

Nice changes, left some additional improvements 👍🏻

craicoverflow · 2021-02-19T14:15:13Z

docs/commands/rhoas_kafka_list.adoc

      --limit int       The maximum number of Kafka instances to be returned (default 100)
  -o, --output string   Format in which to display the Kafka instances. Choose from: "json", "yml", "yaml"
      --page int        Display the Kafka instances from the specified page number.
+      --search string   Text search to filter the Kafka instances


It should be clear that this searches all fields.

Or rather, it should say which fields are searched.

craicoverflow · 2021-02-19T14:21:46Z

pkg/cmd/kafka/list/list.go

+	var queryString string
+
+	if search != "" {
+		queryString = fmt.Sprintf(


I would also have search on the ID field.

The kafka list endpoint doesn't support searching through the id column.

Failed to parse search query: Unsupported column name for search: 'id'. Supported column names are: region, name, cloud_provider, status, owner. Query invalid: id like 10

Fair enough, ignore me so :)

craicoverflow · 2021-02-19T14:22:16Z

pkg/cmd/kafka/list/list.go

 	a := api.Kafka().ListKafkas(context.Background())
 	a = a.Page(strconv.Itoa(opts.page))
 	a = a.Size(strconv.Itoa(opts.limit))
+	a = a.Search(buildQuery(opts.search))


Suggested change

a = a.Search(buildQuery(opts.search))

if opts.search != "" {

a = a.Search(buildQuery(opts.search))

}

I also think this is a great place to have a debug log of the search query :)

craicoverflow · 2021-02-19T14:25:35Z

locales/cmd/kafka/list/active.en.toml


+[kafka.list.flag.search]
+description = 'Description for the --search flag'
+one = 'Text search to filter the Kafka instances'


It should be clear on which fields it will search.

Should we make it something like Text search to filter the kafka instances by name, owner, cloud_provider, region and status.
Can you suggest something better?

No I think that sounds good!

craicoverflow · 2021-02-22T12:44:56Z

Needs rebase. Can't verify yet until staging is fixed.

rkpattnaik780 · 2021-02-22T13:30:38Z

Needs rebase. Can't verify yet until staging is fixed.

Rebased

craicoverflow · 2021-02-22T14:54:17Z

❯ rhoas kafka list --search "enda test"
Error: Failed to parse search query: Unable to list kafka requests for ephelan_kafka_devexp: MGD-SERV-API-23: Failed to parse search query: Provided search query seems incomplete: 'name like %enda test% or owner like %enda test% or cloud_provider like %enda test% or region like %enda test% or status like %enda test%'

Having a space in the query throws an API error. It is best to validate this client-side.

We could throw an error in the CLI if the search query has spaces? What do you think?

rkpattnaik780 · 2021-02-22T15:00:01Z

❯ rhoas kafka list --search "enda test"
Error: Failed to parse search query: Unable to list kafka requests for ephelan_kafka_devexp: MGD-SERV-API-23: Failed to parse search query: Provided search query seems incomplete: 'name like %enda test% or owner like %enda test% or cloud_provider like %enda test% or region like %enda test% or status like %enda test%'
Having a space in the query throws an API error. It is best to validate this client-side.

We could throw an error in the CLI if the search query has spaces? What do you think?

Yes, that will be better. I will look into it.
Do you think we should throw errors for other unsupported chars as well, the server expects search values to conform to '^([a-zA-Z0-9-_%]*[a-zA-Z0-9-_%])?$', maybe we should use this one for validation.
What do you think @craicoverflow ?

Search flag has been added to kafka list to filter by text. fixes #181

craicoverflow

Left a couple of final optional changes you could make, but they are nitpicks. Approving! 👍🏻

craicoverflow · 2021-02-23T11:05:38Z

pkg/cmd/kafka/list/list.go

+
+	if opts.search != "" {
+
+		if err = kafka.ValidateSearchInput(opts.search); err != nil {


Usually we do this validation inside RunE (as early as possible)

craicoverflow · 2021-02-23T11:07:23Z

locales/kafka/active.en.toml

+[kafka.validation.error.invalidSearchValue]
+description = 'Error message when invalid search input is provided'
+one = '''
+Illegal search value "{{.Search}}", search input must satisfy the following conditions:


Error messages should begin with a lowercase in Go. This is because it will usually be prefixed with something like "Error: ":

Suggested change

Illegal search value "{{.Search}}", search input must satisfy the following conditions:

illegal search value "{{.Search}}", search input must satisfy the following conditions:

craicoverflow · 2021-02-23T11:08:10Z

pkg/kafka/kafka_util.go

 	return kafka
 }
+
+func ValidateSearchInput(val interface{}) error {


You should add a comment documenting the purpose of this function

rkpattnaik780 requested a review from craicoverflow February 19, 2021 13:03

craicoverflow suggested changes Feb 19, 2021

View reviewed changes

rkpattnaik780 force-pushed the kafka_list_search branch from 1d3d2d6 to 987f1c7 Compare February 22, 2021 10:44

rkpattnaik780 requested a review from craicoverflow February 22, 2021 10:54

rkpattnaik780 force-pushed the kafka_list_search branch from 7dc34db to 8889a1e Compare February 22, 2021 12:55

rkpattnaik780 added 4 commits February 23, 2021 00:20

feat(kafka list): add search flag

58af1e1

Search flag has been added to kafka list to filter by text. fixes #181

Chore: change helper texts and add debugger

a72a00b

fix: typo in kafka list search debug message

f08c864

Chore: validate search value against RegEx

8b122a8

rkpattnaik780 force-pushed the kafka_list_search branch from 8889a1e to 8b122a8 Compare February 22, 2021 18:54

craicoverflow approved these changes Feb 23, 2021

View reviewed changes

Chore: handle search input validation in RunE

dcdb517

rkpattnaik780 force-pushed the kafka_list_search branch from 18367f8 to dcdb517 Compare February 23, 2021 11:51

rkpattnaik780 merged commit 7e2b9c1 into main Feb 23, 2021

rkpattnaik780 deleted the kafka_list_search branch February 23, 2021 12:10

rkpattnaik780 mentioned this pull request Oct 4, 2021

docs: added --search flag #1167

Closed

9 tasks


		if opts.search != "" {

		if err = kafka.ValidateSearchInput(opts.search); err != nil {

	Illegal search value "{{.Search}}", search input must satisfy the following conditions:
	illegal search value "{{.Search}}", search input must satisfy the following conditions:

Conversation

rkpattnaik780 commented Feb 19, 2021

Uh oh!

craicoverflow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

craicoverflow commented Feb 22, 2021

Uh oh!

rkpattnaik780 commented Feb 22, 2021

Uh oh!

craicoverflow commented Feb 22, 2021

Uh oh!

rkpattnaik780 commented Feb 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

craicoverflow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rkpattnaik780 commented Feb 22, 2021 •

edited

Loading