Adding input validations for subcommands of rhoas service-registry list#1163
Adding input validations for subcommands of rhoas service-registry list#1163rkpattnaik780 merged 3 commits intoredhat-developer:mainfrom
Conversation
amoghrajesh
force-pushed
the
amogh-patch
branch
from
238aa4b to
b70f0c7
Compare
rkpattnaik780
left a comment
rkpattnaik780
left a comment
There was a problem hiding this comment.
Thanks a lot for this PR @amoghrajesh !
While a common method to handle flag inputs is a good approach in the long run, this should have validations like kafka topic list for now.
|
@rkpattnaik780 I have addressed the review comments, could you have a look again? |
pkg/cmd/registry/list/list.go
Outdated
| return flag.InvalidValueError("output", opts.outputFormat, flagutil.ValidOutputFormats...) | ||
| } | ||
| if opts.page < 1 { | ||
| return opts.localizer.MustLocalizeError("registry.list.flag.page", |
There was a problem hiding this comment.
Use of if looks great, however the error message returned should be changed. Current message doesn't tell about the error, rather simply displays the flag description.
There was a problem hiding this comment.
@rkpattnaik780 I don't understand how the rendering is happening for opts.localizer.MustLocalizeError. I want to know how its parameters work. Could you guide me in the same?
There was a problem hiding this comment.
@amoghrajesh We will take care of that. @rkpattnaik780 will take your PR and do all required changes and ping you with info how we fixed them
There was a problem hiding this comment.
@wtrocki got it. Could you merge this or label it so it can be counted as a Hactoberfest PR for me?
There was a problem hiding this comment.
Yes. It will be merged today! I promise. Thank you so much for your contribution!
There was a problem hiding this comment.
Hi @amoghrajesh you can refer to this commit to understand more about how this project implements localization. Basically we keep all the localized strings here.
Hope this helps :)
Thanks!
wtrocki
left a comment
wtrocki
left a comment
There was a problem hiding this comment.
Amazing contribution. Thank you so much!
I think we can take it over from there and get this merged
There was a problem hiding this comment.
Thanks a lot for this PR @amoghrajesh . Great job. 💯
|
@amoghrajesh If you check 3d2ca33 it has good example how to create keys. Our team needs to document that better in contributing guide. Sounds like another issue we can create @rkpattnaik780 ? |
Thanks for pointing it out @wtrocki . That will indeed be a great addition. |
3 participants
Description:
This PR adds support for sanity validation of the input received for the service-registry commands, specifically --limit and --page. These two are checked to have a value of >0 so that the request doesn't need to go through and an early failure can be reported.
Closes #1111
Type of change
Checklist