Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Jul 9, 2021

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