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

CBAPI-5258: Implement audit log search API #500

Merged
merged 6 commits into from
Apr 19, 2024

Conversation

abowersox-cb
Copy link
Contributor

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Tests have been added that prove the fix is effective or that the feature works.
  • New and existing tests pass locally with the changes.
  • Code follows the style guidelines of this project (PEP8, clean code, linter).
  • A self-review of the code has been done.

What is the ticket or issue number?

https://jira.carbonblack.local/browse/CBAPI-5258

Pull Request Description

Implemented audit log search by using a query object in the standard pattern. Query object supports standard criteria and exclusions and async queries.

Does this introduce a breaking change?

  • Yes
  • No

How Has This Been Tested?

Unit tests have been written; test coverage at 97%.

@staticmethod
def _create_valid_time_filter(kwargs):
"""
Verifies that an alert criteria key has the timerange functionality
Copy link
Contributor

Choose a reason for hiding this comment

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

alert criteria ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's why this was still only a draft PR, because I wanted one more chance to go over all the docstrings. :)

from cbc_sdk.platform.audit import AuditLog
from cbc_sdk.platform.audit import AuditLogRecord

from cbc_sdk.platform.audit import AuditLogRecord as AuditLog
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to rename this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the name of the class is misleading; it was AuditLog but it really represents a single record in the audit log, hence, AuditLogRecord. The old name must be kept around, however, for backward compatibility.

Copy link
Contributor

Choose a reason for hiding this comment

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

AuditLog is both plural and singular. It is an Audit Log

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, you're saying that the class shouldn't be renamed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I am suggesting we don't rename the class unless we have a functional reason why we need a different name

@@ -39,3 +67,275 @@ def get_auditlogs(cb):
"""
res = cb.get_object("/integrationServices/v3/auditlogs")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a plan to replace this with _queue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that would be story CBAPI-5259.

@abowersox-cb abowersox-cb marked this pull request as ready for review April 18, 2024 19:37
@abowersox-cb abowersox-cb merged commit 8c12f67 into feature-audit-logs Apr 19, 2024
4 checks passed
@abowersox-cb abowersox-cb deleted the CBAPI-5258_audit_log_search branch April 19, 2024 19: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.

3 participants