Skip to content

feat(consumer-group list): add search flag#813

Merged
rkpattnaik780 merged 3 commits intomainfrom
consumer_group_filter
Jul 9, 2021
Merged

feat(consumer-group list): add search flag#813
rkpattnaik780 merged 3 commits intomainfrom
consumer_group_filter

Conversation

@rkpattnaik780
Copy link
Contributor

Add --search flag for consumer group list.

Closes #714

Verification Steps

  1. Create consumer groups for a Kafka instance.
  2. Run consumer group list command with search flag.
go run ./cmd/rhoas kafka consumer-group list --search console-consume -o yaml
  1. It should return the consumer groups.

Type of change

  • 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 not work as expected)
  • Documentation change
  • Other (please specify)

Checklist

  • Documentation added for the feature
  • CI and all relevant tests are passing
  • Code Review completed
  • Verified independently by reviewer

"github.com/redhat-developer/app-services-cli/pkg/localize"
)

const (
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be correct regexp? ( alphanumeric and hyphen).

Copy link
Collaborator

@wtrocki wtrocki Jul 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Asked in like that contains regexp. For me this looks reasonable, but lets see if that is aligned with backend

)

const (
legalSearchChars = "^[a-zA-Z0-9-]+$"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What validation for search you have in backend @sknot-rh @MikeEdgar ?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which search filter does this apply to? The topic query param?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is for group-id-filter param.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rkpattnaik780 , @wtrocki - there is currently no validation for that param.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wtrocki @MikeEdgar @craicoverflow should we restrict special characters for this argument? Passing * gets into filesystem.
Screenshot from 2021-07-09 12-53-06

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@craicoverflow turns out validators wont prevent us from this glitch, user needs to use double quotes. Shall I get rid of validation here altogether?
Screenshot from 2021-07-09 16-31-47

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see. yes do, thanks.

@rkpattnaik780 rkpattnaik780 force-pushed the consumer_group_filter branch from 7323457 to d05b88e Compare July 9, 2021 13:31
@rkpattnaik780 rkpattnaik780 merged commit 3c52d24 into main Jul 9, 2021
@rkpattnaik780 rkpattnaik780 deleted the consumer_group_filter branch July 9, 2021 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add search flag for kafka consumergroup list

4 participants