-
Notifications
You must be signed in to change notification settings - Fork 528
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
chore: use docs-dev
instead of docs
dir for docs
#4522
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm. Other than the ci, it would be good to run the make test-cli command.
Enhancement - GuidelinesThese guidelines serve as a reminder set of considerations when addressing adding a feature to the code. Documentation and Context
Code Standards and Practices
Testing
Additional Checks
|
detection_rules/docs.py
Outdated
@@ -24,6 +24,8 @@ | |||
from .rule_loader import DeprecatedCollection, RuleCollection | |||
from .utils import load_etc_dump, save_etc_dump | |||
|
|||
DOCS_DIR = "docs-dev" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The entire file, is about generating docs in security-docs repository.
Which i feel they are not using for 9.0.0, why are we introducing new directory structure here ?
the package directory being referenced here is for https://github.com/elastic/security-docs/tree/8.17/docs/detections/prebuilt-rules/downloadable-packages
rule details are here https://github.com/elastic/security-docs/tree/8.17/docs/detections/prebuilt-rules/rule-details
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you for spotting this! should be fixed now!
16bce3d
to
425422a
Compare
Test-cli command has correct expected output 👍 Details
|
@@ -24,6 +24,8 @@ | |||
from .rule_loader import DeprecatedCollection, RuleCollection | |||
from .utils import load_etc_dump, save_etc_dump | |||
|
|||
REPO_DOCS_DIR = "docs-dev" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's better to have the constant to use instead of inline values sprinkled through the code (that's exactly the reason I got confused with all "docs" directories)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont this constant is in need for this particular file. This is all external references to security-docs repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Peer review 👍
3753d6e
to
adc902c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@traut Just noticed that we also need to update references in the workflows like attack-coverage-update.yml
We probably need to just do a deeper search for "docs" references.
Can we test with |
Gist update works as expected 🟢 Details
detection-rules on adjust-docs-path-in-code [?] is v0.4.18 via v3.12.9 (detection-rules-build) on eric.forte took 9s
❯ python -m detection_rules dev update-navigator-gists --update-coverage
█▀▀▄ ▄▄▄ ▄▄▄ ▄▄▄ ▄▄▄ ▄▄▄ ▄▄▄ ▄▄▄ ▄ ▄ █▀▀▄ ▄ ▄ ▄ ▄▄▄ ▄▄▄
█ █ █▄▄ █ █▄▄ █ █ █ █ █ █▀▄ █ █▄▄▀ █ █ █ █▄▄ █▄▄
█▄▄▀ █▄▄ █ █▄▄ █▄▄ █ ▄█▄ █▄█ █ ▀▄█ █ ▀▄ █▄▄█ █▄▄ █▄▄ ▄▄█
Token:
Updated ATT&CK coverage URL(s) in /home/forteea1/Code/detection-rules/docs-dev/ATT&CK-coverage.md
Gist update status on 139 files: 200 OK
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
thank you for helping out here, @eric-forte-elastic 🙇 |
ec42623
to
b506e21
Compare
Pull Request
Issue link(s):
Summary - What I changed
Path used for docs in the code updated from
docs
todocs-dev
Checklist
bug
,enhancement
,schema
,maintenance
,Rule: New
,Rule: Deprecation
,Rule: Tuning
,Hunt: New
, orHunt: Tuning
so guidelines can be generatedmeta:rapid-merge
label if planning to merge within 24 hoursContributor checklist