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.

Sign up for GitHub

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

Jump to bottom

fix(serviceaccount): add service account input validation #512

Merged
merged 1 commit into from
Mar 30, 2021
Merged

fix(serviceaccount): add service account input validation #512

merged 1 commit into from
Mar 30, 2021

Conversation

craicoverflow
Copy link
Contributor

@craicoverflow craicoverflow commented Mar 29, 2021 •
edited
Loading

Description

fixes #505

Verification Steps

Run rhoas serviceaccount create commands and enter various values from the rules below to test validation on name and description fields. Do this in both flags and interactive prompt.

Name

  • only lowercase letters (a-z), numbers, and "-" are accepted
  • min length: 1, max length: 50

Description

  • only letters (Aa-Zz) and numbers are accepted
  • max length: 255

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)

Checklist

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

@craicoverflow craicoverflow force-pushed the serviceaccount-validation branch from 36d5a92 to d2a5171 Compare March 29, 2021 12:11
@craicoverflow craicoverflow force-pushed the serviceaccount-validation branch 2 times, most recently from f4a3e21 to 5d41de5 Compare March 29, 2021 13:14
rkpattnaik780
rkpattnaik780 requested changes Mar 30, 2021
View reviewed changes
Copy link
Contributor

@rkpattnaik780 rkpattnaik780 left a comment

Choose a reason for hiding this comment

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

Should we put exact args to be zero for this command, it can be bit confusing

pkg/cmd/serviceaccount/create/create.go Outdated
@@ -199,7 +210,7 @@ func runInteractivePrompt(opts *Options) (err error) {
Help: localizer.MustLocalizeFromID("serviceAccount.create.input.name.help"),
}

err = survey.AskOne(promptName, &opts.name, survey.WithValidator(survey.Required))
err = survey.AskOne(promptName, &opts.name, survey.WithValidator(survey.Required), survey.WithValidator(validation.ValidateDescription))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
err = survey.AskOne(promptName, &opts.name, survey.WithValidator(survey.Required), survey.WithValidator(validation.ValidateDescription))
err = survey.AskOne(promptName, &opts.name, survey.WithValidator(survey.Required), survey.WithValidator(validation.ValidateName))

@craicoverflow
Copy link
Contributor Author

Should we put exact args to be zero for this command, it can be bit confusing

Sure, what was confusing about it? Was it something related to this code change?

@rkpattnaik780
Copy link
Contributor

Should we put exact args to be zero for this command, it can be bit confusing

Sure, what was confusing about it? Was it something related to this code change?

Nothing related to these changes. I tried doing something like ./rhoas serviceaccount create <name> and it started the interactive mode, which could be bit confusing to end user. We could do it in a separate PR as well. What do you think?

@craicoverflow
Copy link
Contributor Author

Should we put exact args to be zero for this command, it can be bit confusing

Sure, what was confusing about it? Was it something related to this code change?

Nothing related to these changes. I tried doing something like ./rhoas serviceaccount create <name> and it started the interactive mode, which could be bit confusing to end user. We could do it in a separate PR as well. What do you think?

Hmm, since it is so small, I will apply it now.

@craicoverflow craicoverflow force-pushed the serviceaccount-validation branch 3 times, most recently from b1856d8 to 13a3080 Compare March 30, 2021 08:22
@craicoverflow craicoverflow force-pushed the serviceaccount-validation branch from 36dc67b to ec76591 Compare March 30, 2021 08:39
@wtrocki
Copy link
Collaborator

wtrocki commented Mar 30, 2021

@craicoverflow would this PR also solve this: #515 or this is separate?

@craicoverflow
Copy link
Contributor Author

@craicoverflow would this PR also solve this: #515 or this is separate?

Separate.

rkpattnaik780
rkpattnaik780 reviewed Mar 30, 2021
View reviewed changes
pkg/cmd/serviceaccount/create/create.go
@@ -50,7 +52,8 @@ func NewCreateCommand(f *factory.Factory) *cobra.Command {
Short: localizer.MustLocalizeFromID("serviceAccount.create.cmd.shortDescription"),
Long: localizer.MustLocalizeFromID("serviceAccount.create.cmd.longDescription"),
Example: localizer.MustLocalizeFromID("serviceAccount.create.cmd.example"),
RunE: func(cmd *cobra.Command, _ []string) error {
Args: cobra.NoArgs,
Copy link
Contributor

Choose a reason for hiding this comment

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

We should change cobra.ExactArgs(0) to cobra.NoArgs throughout the app in a follow up PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. thanks!

@craicoverflow craicoverflow merged commit 199e5f3 into main Mar 30, 2021
@craicoverflow craicoverflow deleted the serviceaccount-validation branch March 30, 2021 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rkpattnaik780 rkpattnaik780 rkpattnaik780 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CLI should validate name provided for service account
3 participants
@craicoverflow @rkpattnaik780 @wtrocki