-
Notifications
You must be signed in to change notification settings - Fork 43
Add support for Sysdig Secure compliance tasks #77
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
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 besides a couple of comments. Nice work, thank you for the refactor.
sdcclient/_secure.py
Outdated
if scope is not None: | ||
task["scope"] = scope | ||
if enabled is not None: | ||
task["enabled"] = enabled |
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've seen this a couple of times at the code. Would this be worthy?
options = {
'name': name,
'moduleName': module_name,
...
}
for key, value in options.items():
if value is not None:
task[key] = value
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'm not sure how readable are dict completions, but this is a try: 33e82b2
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.
Yeah, totally right. Wasn't sure at all, but wanted to know your opinion ;-)
Let's keep python2 compatibility.
@meskio unfortunately this time the build was failing for real:
Would you mind taking a look and see if you can file another PR to fix the test (or code)? Let me know if you need any help. Thanks! |
Ouch, I thought I tested. I'm looking into that. |
As well as adding support to fetch policy events by id.