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

[FR] Add Env Var DR_CLI_MAX_WIDTH and DaC Docs Updates #4518

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

[FR] Add Env Var DR_CLI_MAX_WIDTH and DaC Docs Updates #4518

wants to merge 12 commits into from
+80 −9

Conversation

eric-forte-elastic
Copy link
Contributor

@eric-forte-elastic eric-forte-elastic commented Mar 4, 2025 •
edited
Loading

Pull Request

Issue link(s):
Resolves #4517

Summary - What I changed

The current help text gets cut off in the Click CLI as a byproduct of Click's default settings. I have added an env variable DR_CLI_MAX_WIDTH that can be used to change the default value to a larger number to prevent the text from being cutoff. Additionally I have increased the value from the default value in order to prevent the text from being cutoff

How To Test

Run the following commands and see that the text is no longer cutoff

python3 -m detection_rules --help

python3 -m detection_rules dev --help

Expected Output:

click_cli_output

Checklist

  • Added a label for the type of pr: bug, enhancement, schema, maintenance, Rule: New, Rule: Deprecation, Rule: Tuning, Hunt: New, or Hunt: Tuning so guidelines can be generated
  • Added the meta:rapid-merge label if planning to merge within 24 hours
  • Secret and sensitive material has been managed correctly
  • Automated testing was updated or added to match the most common scenarios
  • Documentation and comments were added for features that require explanation

Contributor checklist

@eric-forte-elastic eric-forte-elastic added documentation Improvements or additions to documentation enhancement New feature or request cli command line tooling python Internal python for the repository detections-as-code patch labels Mar 4, 2025
@eric-forte-elastic eric-forte-elastic self-assigned this Mar 4, 2025
@eric-forte-elastic eric-forte-elastic linked an issue Mar 4, 2025 that may be closed by this pull request
[Bug] DaC detection_rules help texts are cut off #4517
Open
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Mar 4, 2025

Enhancement - Guidelines

These guidelines serve as a reminder set of considerations when addressing adding a feature to the code.

Documentation and Context

  • Describe the feature enhancement in detail (alternative solutions, description of the solution, etc.) if not already documented in an issue.
  • Include additional context or screenshots.
  • Ensure the enhancement includes necessary updates to the documentation and versioning.

Code Standards and Practices

  • Code follows established design patterns within the repo and avoids duplication.
  • Code changes do not introduce new warnings or errors.
  • Variables and functions are well-named and descriptive.
  • Any unnecessary / commented-out code is removed.
  • Ensure that the code is modular and reusable where applicable.
  • Check for proper exception handling and messaging.

Testing

  • New unit tests have been added to cover the enhancement.
  • Existing unit tests have been updated to reflect the changes.
  • Provide evidence of testing and validating the enhancement (e.g., test logs, screenshots).
  • Validate that any rules affected by the enhancement are correctly updated.
  • Ensure that performance is not negatively impacted by the changes.
  • Verify that any release artifacts are properly generated and tested.

Additional Checks

  • Ensure that the enhancement does not break existing functionality.
  • Review the enhancement with a peer or team member for additional insights.
  • Verify that the enhancement works across all relevant environments (e.g., different OS versions).
  • Confirm that all dependencies are up-to-date and compatible with the changes.
  • Confirm that the proper version label is applied to the PR patch, minor, major.

shashank-elastic
shashank-elastic approved these changes Mar 4, 2025
View reviewed changes
Copy link
Contributor

@shashank-elastic shashank-elastic left a comment

Choose a reason for hiding this comment

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

Locally tested the command with the variable. Works as expected

Mikaayenson
Mikaayenson reviewed Mar 4, 2025
View reviewed changes
README.md
@@ -133,6 +133,10 @@ For more advanced command line interface (CLI) usage, refer to the [CLI guide](C

We welcome your contributions to Detection Rules! Before contributing, please familiarize yourself with this repository, its [directory structure](#overview-of-this-repository), and our [philosophy](PHILOSOPHY.md) about rule creation. When you're ready to contribute, read the [contribution guide](CONTRIBUTING.md) to learn how we turn detection ideas into production rules and validate with testing.

## Detections as Code (DaC)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add it to the table like in @traut Cortado 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.

Good call! Updated

@eric-forte-elastic eric-forte-elastic changed the title [FR] Add Env Var DR_CLI_MAX_WIDTH [FR] Add Env Var DR_CLI_MAX_WIDTH and DaC Docs Updates Mar 4, 2025
approksiu
approksiu reviewed Mar 5, 2025
View reviewed changes
Copy link

@approksiu approksiu left a comment

Choose a reason for hiding this comment

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

Could we change "custom rules.md" to something more related to dac rule management?

README.md
## Detections as Code (DaC)

The Detection Rules repo includes a number of commands to help one manage rules with an "as code" philosophy. We recommend starting with our [DaC Specific Documentation](https://dac-reference.readthedocs.io/en/latest/) for strategies and recommended setup information. However, if you would prefer to jump right in, please see our [custom rules documentation](docs/custom-rules.md) for information on how to configure this repo for use with custom rules followed by our [CLI documentation](CLI.md) for information on our commands to import and export rules.

Copy link

@approksiu approksiu Mar 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Did you want to change the custom-rules page name to more related to dac?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would renaming it to dac-custom-rules.md alleviate your concern? My fear is that there may be customers looking for just setting up custom rules rather than a full dac pipeline, and those users may not know where to go for information.

Or perhaps something like detections-as-code-custom-rules?

Alternatively, I we could change the name to dac.md or detections-as-code.md and we retain a header in that file called Custom Rules so folks would have something find for this.

Thanks!

Copy link

Choose a reason for hiding this comment

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

Thanks @eric-forte-elastic for the detailed discussion yesterday, let's call this page custom-rules-management.md, and mention dac on the page itself.

approksiu
approksiu reviewed Mar 6, 2025
View reviewed changes
CLI.md
@@ -281,7 +284,7 @@ Options:
-id, --rule-id TEXT
-r, --replace-id Replace rule IDs with new IDs before export
-h, --help Show this message and exit.
(detection-rules-build) (base) ➜ detection-rules git:(rule-loader) ✗
(detection-rules-build) (base) ➜ detection-rules git:(main) ✗
```

Alternatively, rules can be exported into a consolidated ndjson file which can be imported in the Kibana security app
Copy link

Choose a reason for hiding this comment

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

Please add sub-header to highlight this command.

Copy link

Choose a reason for hiding this comment

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

Would it make sense to move the upload-rule command below and make it more clear not to use it? Is it removed in 9.0?

approksiu
approksiu requested changes Mar 6, 2025
View reviewed changes
CLI.md
@@ -281,7 +284,7 @@ Options:
-id, --rule-id TEXT
-r, --replace-id Replace rule IDs with new IDs before export
-h, --help Show this message and exit.
(detection-rules-build) (base) ➜ detection-rules git:(rule-loader) ✗
(detection-rules-build) (base) ➜ detection-rules git:(main) ✗
```

Alternatively, rules can be exported into a consolidated ndjson file which can be imported in the Kibana security app
Copy link

Choose a reason for hiding this comment

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

Would it make sense to move the upload-rule command below and make it more clear not to use it? Is it removed in 9.0?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mikaayenson Mikaayenson Mikaayenson approved these changes

@approksiu approksiu approksiu requested changes

@shashank-elastic shashank-elastic shashank-elastic approved these changes

@terrancedejesus terrancedejesus terrancedejesus approved these changes

Requested changes must be addressed to merge this pull request.

Assignees

@eric-forte-elastic eric-forte-elastic

Labels
backport: auto cli command line tooling detections-as-code documentation Improvements or additions to documentation enhancement New feature or request patch python Internal python for the repository
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug] DaC detection_rules help texts are cut off
5 participants
@eric-forte-elastic @Mikaayenson @approksiu @shashank-elastic @terrancedejesus