Skip to content

Conversation

@alexiskulash
Copy link

Description

This provides the basic functionality to delete a rule by specifying an ID. In the future, could use the following improvements:

  • Validation of the rule ID provided
  • Confirmation from user before proceeding with rule deletion
  • Bulk deletes by specifying a comma-separated list (or CSV)
  • Maybe even deletion by rule name?

@alexiskulash alexiskulash requested review from cyx and vprasanth January 26, 2021 00:21
@alexiskulash alexiskulash self-assigned this Jan 26, 2021
},
}

cmd.Flags().StringVarP(&flags.id, "id", "i", "", "ID of the rule to delete (required)")
Copy link
Contributor

Choose a reason for hiding this comment

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

Wonder if not having a short form given id is already super short is fine here?

Copy link
Author

Choose a reason for hiding this comment

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

I was thinking about that... "id" wasn't accepted as that param, so I suppose I just need to leave it as an empty string and perhaps that'll omit the short form? 🤞

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.

Have a question re: StringVarP vs StringVar for the command line flag, but otherwise :shipit:

@alexiskulash
Copy link
Author

Have a question re: StringVarP vs StringVar for the command line flag, but otherwise :shipit:

image

@alexiskulash alexiskulash merged commit d3c341c into main Jan 26, 2021
@alexiskulash alexiskulash deleted the add/delete-rule branch January 26, 2021 00:41
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.

👍 thanks!

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.

3 participants