feat(kafka topic list): add flags for pagination by rkpattnaik780 · Pull Request #810 · redhat-developer/app-services-cli

rkpattnaik780 · 2021-07-07T14:30:00Z

add --page and --size flag to kafka topic list command so user can navigate to different pages and adjust capacity of a page.

Addresses #766

Verification Steps

Run the kafka list command with page and size flag.

go run ./cmd/rhoas kafka topic list --page 1 --size 1

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

bhardesty · 2021-07-07T14:34:07Z

pkg/localize/locales/en/cmd/kafka_topic_list.en.toml


+[kafka.topic.list.flag.page.description]
+description = 'Description for the --page flag'
+one = 'Current page number for list of topics'


Suggested change

one = 'Current page number for list of topics'

one = 'Current page number for list of topics.'

bhardesty · 2021-07-07T14:34:38Z

pkg/localize/locales/en/cmd/kafka_topic_list.en.toml

+
+[kafka.topic.list.flag.size.description]
+description = 'Description for the --size flag'
+one = 'Maximum number of items to be returned per page'


Suggested change

one = 'Maximum number of items to be returned per page'

one = 'Maximum number of items to be returned per page.'

wtrocki · 2021-07-07T14:39:26Z

Good from technical standpoint. Does that works against stage?

rkpattnaik780 · 2021-07-07T15:43:16Z

Good from technical standpoint. Does that works against stage?

Yes it does, I will fix the build and re-request reviews.

craicoverflow

Some changes needed.

craicoverflow · 2021-07-08T11:47:38Z

pkg/cmd/kafka/topic/list/list.go

+const (
+	defaultPage = 1
+	defaultSize = 10
+)


I think we should extract these int a common place which can be used by ALL list commands. Maybe even as a build variable?

You could assign these as the default values when creating Options.

Had to deviate from our current approach as vs code was showing multiple lint warnings/errors, do you want me to fallback to conventional method.

What were the lint errors?

Something like avoid magic numbers, create a named constant.

I don't see them anymore though, build seems to pass as well.

You can created a named var in the build package and reference this instead? Something like build.DefaultPageSize

This does not seem a very good idea. Other build variables seem to have a different context than stuffs like default values for page and size. Can we put it at some level lower than build?

You are right, and yes we can! But we can still make it externally configurable without having it be specifically in build. So wherever you decide to put it, make it configurable in the same way

Have put them in cmdutil package, let me know what you think.

craicoverflow · 2021-07-08T11:56:33Z

pkg/cmd/kafka/topic/list/list.go

 				}
 			}

+			validator := &topicutil.Validator{


Suggested change

validator := &topicutil.Validator{

validator := topicutil.Validator{

The pointer is not needed here.

craicoverflow · 2021-07-08T11:56:44Z

pkg/cmd/kafka/topic/list/list.go

+			}
+
 			if opts.search != "" {
 				validator := &topicutil.Validator{


Suggested change

validator := &topicutil.Validator{

validator := topicutil.Validator{

Not needed.

craicoverflow · 2021-07-08T11:58:04Z

pkg/cmd/kafka/topic/list/list.go

+	if opts.size != defaultSize {
+		a = a.Size(int32(opts.size))
+	}


I don't see where defaultSize ever gets used by the API client?

craicoverflow · 2021-07-08T11:58:13Z

pkg/cmd/kafka/topic/list/list.go

+		a = a.Size(int32(opts.size))
+	}
+
+	if opts.page != defaultPage {


I don't see where defaultPage ever gets used by the API client?

These defaults are returned by the response when we don't set flags manually.

Should we let go of that check?

If the backend changes the default size to 20 for example, the CLI will continue to use 10 as the default value. The user may enter --size=10 as the page size, but pagination will not be effective because the comparison is happening against the local defaultSize value.

I think we should have a single, common defaultSize which can be used in control plane and data plane APIs. This gives the CLI more control over pagination defaults and can ensure consistency when different APIs have different defaults.

Make sense?

Also - think about upstream. Default value of 10 is an implementation detail of RHOAS service APIs, but if this CLI could be reused within a different environment upstream those defaults could be completely different.

craicoverflow · 2021-07-08T11:58:29Z

pkg/cmd/kafka/topic/list/list.go

 	kafkaID string
 	output  string
 	search  string
+	page    int


Suggested change

page int

page int32

craicoverflow · 2021-07-08T11:58:36Z

pkg/cmd/kafka/topic/list/list.go

 	output  string
 	search  string
+	page    int
+	size    int


Suggested change

size int

size int32

craicoverflow · 2021-07-08T12:02:28Z

pkg/kafka/topic/validators.go

+
+// ValidatePage validates the value of page flag
+// the valid values can range from [1,...]
+func (v *Validator) ValidatePage(val interface{}) error {


Passing an interface{} and type checking is a bit expensive - we are using it in the other validators as it is a requirement to satisfy the survey interactive requirements..but for pagination we will never use it in such a way so you can pass the known type.

Did this thinking about consistency, we should validate on client side if these flags are greater than zero. Else we'll get a 400 from server.

Yes, we should still validate the page, but no need to pass an interface{}, just the int type will do.

rkpattnaik780 · 2021-07-09T11:41:24Z

Hi @bhardesty. I am seeing some inconsistencies regarding use of periods in flag descriptions. Some flags end with period and some don't. As suggested by @craicoverflow we shouldn't use periods as the flags provided by cobra (--default and --help) don't use them too. Would you like this to be addressed in another issue, we can finalize a convention and fix the descriptions?

craicoverflow · 2021-07-09T15:21:08Z

pkg/cmdutil/constants.go

+	DefaultPageSize = 10
+
+	// DefaultPageNumber is the default page number when using list commands
+	DefaultPageNumber = 1


I think these needs to to be vars to be configurable at build-time.

Build variables can only be a string 🤔 Leave it like this for now and we can figure out how to configure it later (leave a TODO)

Checked now it doesnt work as expected! It needs to be addressed in a follow-up.

craicoverflow · 2021-07-09T15:21:41Z

pkg/cmdutil/constants.go

 	DefaultJSONIndent = "    "
+
+	// DefaultPageSize is the default number of items per page when using list commands
+	DefaultPageSize = 10


In a follow up PR, could you apply these wherever there is pagination?

bhardesty reviewed Jul 7, 2021

View reviewed changes

craicoverflow suggested changes Jul 8, 2021

View reviewed changes

rkpattnaik780 force-pushed the topic_pagination branch from 20727b6 to a667c59 Compare July 8, 2021 13:22

rkpattnaik780 requested review from bhardesty and craicoverflow July 9, 2021 11:33

rkpattnaik780 added 6 commits July 9, 2021 19:06

feat(kafka topic list): add flags for pagination

af47146

fix: address review comments

a1c1b67

docs(kafka topic list): update docs

74269bd

fix: remove validator methods for page and size

462ab64

fix: failing build due to unused vars

01f7350

fix: use build variables

20a2494

rkpattnaik780 force-pushed the topic_pagination branch from 2eecfd5 to 20a2494 Compare July 9, 2021 13:38

rkpattnaik780 added 2 commits July 9, 2021 19:11

fix: typo in makefile

f63d62b

docs: regenerate docs

5d805b2

craicoverflow approved these changes Jul 9, 2021

View reviewed changes

rkpattnaik780 merged commit 432c3b4 into main Jul 12, 2021

rkpattnaik780 deleted the topic_pagination branch July 12, 2021 07:31

rkpattnaik780 mentioned this pull request Jul 21, 2021

Add configurable build variables for pagination #831

Closed

rkpattnaik780 mentioned this pull request Aug 18, 2021

refactor(pagination): use build variables for default pagination values #920

Merged

9 tasks

	one = 'Current page number for list of topics'
	one = 'Current page number for list of topics.'

	one = 'Maximum number of items to be returned per page'
	one = 'Maximum number of items to be returned per page.'

	validator := &topicutil.Validator{
	validator := topicutil.Validator{

Conversation

rkpattnaik780 commented Jul 7, 2021

Verification Steps

Type of change

Checklist

Uh oh!

bhardesty Jul 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wtrocki commented Jul 7, 2021

Uh oh!

rkpattnaik780 commented Jul 7, 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!

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

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!

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!

rkpattnaik780 commented Jul 9, 2021

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

bhardesty Jul 7, 2021 •

edited

Loading

rkpattnaik780 Jul 8, 2021 •

edited

Loading