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

Expand OSSEC documentation based on @emkll's old commits #106

Merged
merged 2 commits into from
Dec 17, 2020

Conversation

buildbricks
Copy link
Contributor

@buildbricks buildbricks commented Nov 18, 2020

Status

Ready for review

Description of Changes

Testing

Release

Checklist (Optional)

  • Doc linting (make docs-lint) passed locally
  • Doc link linting (make docs-linkcheck) passed
  • You have previewed (make docs) docs at http://localhost:8000

@buildbricks
Copy link
Contributor Author

@eloquence

@eloquence eloquence changed the title OSSEC alerts original commits by emkil Expand OSSEC documentation based on @emkll's old commits Dec 1, 2020
@emkll
Copy link
Contributor

emkll commented Dec 1, 2020

Thanks @joaedwar for adding these. I took a pass through this and clarified some original language, and appended a commit to your branch. I think this is ready for a final review/merge.

Copy link
Member

@eloquence eloquence left a comment

Choose a reason for hiding this comment

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

The biggest issue is that this PR removes valid instructions that were recently added, likely due to a merge issue when applying the old changes. In addition, I'd appreciate a sanity check on the specific alerts I'm not seeing in my own alert inbox/logs.

docs/ossec_alerts.rst Outdated Show resolved Hide resolved
docs/ossec_alerts.rst Outdated Show resolved Hide resolved
docs/ossec_alerts.rst Outdated Show resolved Hide resolved
docs/ossec_alerts.rst Outdated Show resolved Hide resolved
docs/ossec_alerts.rst Show resolved Hide resolved
docs/ossec_alerts.rst Show resolved Hide resolved
docs/ossec_alerts.rst Outdated Show resolved Hide resolved
@emkll emkll force-pushed the ossec_alerts_reuse branch 2 times, most recently from c677528 to 4152932 Compare December 3, 2020 22:36
@emkll
Copy link
Contributor

emkll commented Dec 3, 2020

Good catch @eloquence , I've restored the next that was lost in the port and addressed your comments, and rebased on latest main

docs/ossec_alerts.rst Outdated Show resolved Hide resolved
docs/ossec_alerts.rst Outdated Show resolved Hide resolved
docs/ossec_alerts.rst Outdated Show resolved Hide resolved
docs/ossec_alerts.rst Outdated Show resolved Hide resolved
docs/ossec_alerts.rst Outdated Show resolved Hide resolved
@emkll emkll force-pushed the ossec_alerts_reuse branch from 4152932 to f457fa2 Compare December 16, 2020 14:17
@emkll
Copy link
Contributor

emkll commented Dec 16, 2020

thanks @rmol for the review, your comments should have been addressed in the latest revision

Copy link
Contributor

@rmol rmol left a comment

Choose a reason for hiding this comment

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

Two more nitpicky comments. Sorry.

docs/ossec_alerts.rst Outdated Show resolved Hide resolved
docs/ossec_alerts.rst Outdated Show resolved Hide resolved
and restore deleted entries.
@emkll emkll force-pushed the ossec_alerts_reuse branch from f457fa2 to 529e744 Compare December 17, 2020 23:26
@rmol rmol dismissed eloquence’s stale review December 17, 2020 23:42

I believe all of the points have been addressed.

@rmol rmol merged commit c1e5b01 into freedomofpress:main Dec 17, 2020
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.

Documentation about OSSEC alert levels
4 participants