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: add name flag support for rules delete command #54

Merged
merged 2 commits into from
Jan 26, 2021
Merged

Conversation

vprasanth
Copy link
Contributor

@vprasanth vprasanth commented Jan 26, 2021

  • ensure id and name flags are mutually exclusive
  • added rule id pattern check
  • fixed bug that didn't report not found rule when id was used
  • added global force flag to skip confirmation prompt

Close A0CLI-40

- ensure id and name flags are mutually exclusive
- added rule id pattern check
- fixed bug that didn't report not found rule when id was used

Close A0CLI-40
@@ -200,12 +203,44 @@ func deleteRulesCmd(cli *cli) *cobra.Command {
Long: `Delete a rule:

auth0 rules delete --id "12345"`,
PreRunE: func(cmd *cobra.Command, args []string) error {
if flags.id != "" && flags.name != "" {
return fmt.Errorf("TMI! 🤯 use either --name or --id")
Copy link
Contributor

Choose a reason for hiding this comment

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

🍭

Comment on lines +214 to +215
ruleIDPattern := "^rul_[A-Za-z0-9]{16}$"
re := regexp.MustCompile(ruleIDPattern)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a big deal since this is a CLI, but typically you put MustCompile lines in vars outside so you don't have to re-do it.

(We can keep it like this for this PR though, just wanted to share that compiling regexp is typically expensive if you're doing server side code).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, that makes sense, good to know, ty!

Copy link
Contributor

@cyx cyx left a comment

Choose a reason for hiding this comment

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

This looks great! Thank you

@vprasanth vprasanth merged commit 1a9e75c into main Jan 26, 2021
@vprasanth vprasanth deleted the a0cli-40 branch January 26, 2021 23:03
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.

2 participants