fix(serviceaccount reset-credentials): validate serviceaccount ID in prompt by rkpattnaik780 · Pull Request #720 · redhat-developer/app-services-cli

rkpattnaik780 · 2021-06-15T13:24:53Z

Client side validation for service account ID in interactive mode.

Closes #716

Verification Steps

Run command to reset credentials for service account in interactive mode.

go run ./cmd/rhoas serviceaccount reset-credentials

Input any random string that is not an UUID, e.g "kafka". It should throw the error:

Sorry, your reply was invalid: "kafka" is not a valid UUID

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

… prompt

rkpattnaik780 · 2021-06-15T13:27:11Z

Should client side validation be extended to non-interactive mode as well?

validateIDFlag := validation.ValidateID(opts.localizer)
validID := validateIDFlag(opts.id)
if validID != nil {
return validID
}

Also, it seems bit complex to write tests for higher order functions being used as validators.

craicoverflow

Should client side validation be extended to non-interactive mode as well?

Yes, we should. Any client-side validation improves UX/DX so this would be nice to have.

Also, it seems bit complex to write tests for higher order functions being used as validators.

Yeah, I see what you mean! Now that we are needing to pass localizer to validators they are no longer simple functions.

What would be better (and you can do this in this PR) is to create a Validator type which contains all the validator functions for service accounts.

craicoverflow · 2021-06-16T07:40:18Z

pkg/serviceaccount/validation/validation.go

 }
+
+// ValidateID validates if ID is a valid UUID
+func ValidateID(localizer localize.Localizer) func(v interface{}) error {


Suggested change

func ValidateID(localizer localize.Localizer) func(v interface{}) error {

func ValidateUUID(localizer localize.Localizer) func(v interface{}) error {

craicoverflow · 2021-06-16T07:41:23Z

pkg/serviceaccount/validation/validation.go

 	legalNameChars = "^[a-z]([-a-z0-9]*[a-z0-9])?$"
 	maxNameLength  = 50
 	minNameLength  = 1
+	leagalUUID     = "^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$"


Suggested change

leagalUUID = "^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$"

legalUUID = "^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$"

craicoverflow · 2021-06-16T13:01:24Z

pkg/serviceaccount/validation/validation.go

 }
+
+// ValidateUUID validates if ID is a valid UUID
+func ValidateUUID(localizer localize.Localizer) func(v interface{}) error {


I think it will be a good idea for readibility if we do not use anonymous return functions anymore for all validators. I'll explain in a follow up issue

PS: It would also be nice to validate the flag for UUId like you had suggested.

It has been done in reset_credentials.go. Did you expect something else?

I think it will be a good idea for readibility if we do not use anonymous return functions anymore for all validators. I'll explain in a follow up issue

Maked #721 as WIP, it has localizations for validators using anonymous functions, should this PR wait for the issue?

Ah I had not installed the binary :)

No you can merge this PR beforehand if you like.

craicoverflow · 2021-06-16T13:03:06Z

pkg/serviceaccount/validation/validation_test.go

 	}
 }

+func TestValidateUUID(t *testing.T) {


craicoverflow · 2021-06-16T13:12:34Z

PS: It would also be nice to validate the flag for UUId like you had suggested.

…prompt (redhat-developer#720)

…prompt (#720)

fix(serviceaccount reset-credentials): validate serviceaccount ID for…

1ac6298

… prompt

rkpattnaik780 requested a review from craicoverflow June 15, 2021 13:24

craicoverflow reviewed Jun 16, 2021

View reviewed changes

test: tests for ValidateUUID

93867b9

rkpattnaik780 requested a review from craicoverflow June 16, 2021 11:08

fix: change method name

9ee8c8a

craicoverflow approved these changes Jun 16, 2021

View reviewed changes

rkpattnaik780 merged commit 0f61916 into develop Jun 16, 2021

rkpattnaik780 deleted the svc_acc_validation branch June 16, 2021 14:07

craicoverflow pushed a commit to craicoverflow/app-services-cli that referenced this pull request Jun 18, 2021

fix(serviceaccount reset-credentials): validate serviceaccount ID in …

33fbc64

…prompt (redhat-developer#720)

craicoverflow mentioned this pull request Jun 18, 2021

fix(serviceaccount reset-credentials): validate serviceaccount ID in … craicoverflow/app-services-cli#1

Closed

9 tasks

rkpattnaik780 mentioned this pull request Jun 18, 2021

test: add tests for validation methods #728

Merged

9 tasks

craicoverflow pushed a commit that referenced this pull request Jun 30, 2021

fix(serviceaccount reset-credentials): validate serviceaccount ID in …

ca0e393

…prompt (#720)

craicoverflow pushed a commit that referenced this pull request Jun 30, 2021

fix(serviceaccount reset-credentials): validate serviceaccount ID in …

b83be2b

…prompt (#720)

craicoverflow pushed a commit that referenced this pull request Jun 30, 2021

fix(serviceaccount reset-credentials): validate serviceaccount ID in …

286be6b

…prompt (#720)

craicoverflow pushed a commit that referenced this pull request Jun 30, 2021

fix(serviceaccount reset-credentials): validate serviceaccount ID in …

169fcdb

…prompt (#720)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(serviceaccount reset-credentials): validate serviceaccount ID in prompt#720

fix(serviceaccount reset-credentials): validate serviceaccount ID in prompt#720
rkpattnaik780 merged 3 commits intodevelopfrom
svc_acc_validation

rkpattnaik780 commented Jun 15, 2021

Uh oh!

rkpattnaik780 commented Jun 15, 2021

Uh oh!

craicoverflow left a comment

Uh oh!

craicoverflow Jun 16, 2021

Uh oh!

craicoverflow Jun 16, 2021

Uh oh!

craicoverflow Jun 16, 2021

Uh oh!

rkpattnaik780 Jun 16, 2021

Uh oh!

rkpattnaik780 Jun 16, 2021

Uh oh!

craicoverflow Jun 16, 2021

Uh oh!

craicoverflow Jun 16, 2021

Uh oh!

craicoverflow Jun 16, 2021

Uh oh!

craicoverflow commented Jun 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	func ValidateID(localizer localize.Localizer) func(v interface{}) error {
	func ValidateUUID(localizer localize.Localizer) func(v interface{}) error {

	leagalUUID = "^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$"
	legalUUID = "^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$"

Conversation

rkpattnaik780 commented Jun 15, 2021

Verification Steps

Type of change

Checklist

Uh oh!

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

craicoverflow commented Jun 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants