Web DLP class/methods #152
Conversation
There was a problem hiding this comment.
Thanks for getting started on this @daxm, really appreciate it!
After addressing those review points we can merge into develop and add some additional work to round it off:
- Docstrings complete with examples
- Adding the docsrc RST files to ensure our readthedocs site will pick up the changes
- Test coverage
Overall great quality pull request and thankyou thankyou thankyou!
| @@ -210,3 +211,19 @@ def vips(self): | |||
|
|
|||
| """ | |||
| return DataCenterVIPSAPI(self) | |||
|
|
|||
| @property | |||
There was a problem hiding this comment.
I think you may have accidentally duplicated this class attribute. We have the admin_and_role_management attribute defined on line 95.
Can you just remove lines 216->221 when you resubmit?
| @@ -8,15 +8,13 @@ class AdminAndRoleManagementAPI(APIEndpoint): | |||
| def add_user(self, name: str, login_name: str, email: str, password: str, **kwargs) -> Box: | |||
There was a problem hiding this comment.
I'm not sure what was changed in this file, just looks like a blank line was removed somewhere. I think we can remove this from the pull request.
pyzscaler/zia/web_dlp.py
Outdated
|
|
||
|
|
||
| class WebDLP(APIEndpoint): | ||
| def get_all(self, **kwargs) -> BoxList: |
There was a problem hiding this comment.
Are you happy if we rename these methods to align with the rest of the SDK?
I've taken the approach that Zscaler may add additional objects to some of these API endpoints, which would mean we need to be a bit more verbose about the verbs and what we're doing. So at the moment it appears that the web_dlp endpoint just has policy rules, but if they were to add anything else then just having zia.web_dlp.get_all() would be ambiguous.
So while it may seem a bit over the top, having a descriptive verb/noun for the methods ensures we don't need to go back and refactor if the API endpoints are expanded. You should see a module.attribute.verb_noun convention used across the SDK.
If we're returning a list of items then we should expect something like zia.web_dlp.list_rules()
pyzscaler/zia/web_dlp.py
Outdated
| """ | ||
| return self._get("webDlpRules") | ||
|
|
||
| def get_item(self, item_id: str) -> Box: |
pyzscaler/zia/web_dlp.py
Outdated
| """ | ||
| return self._get("webDlpRules/lite") | ||
|
|
||
| def post(self, payload: json) -> Box: |
pyzscaler/zia/web_dlp.py
Outdated
| """ | ||
| return self._post("webDlpRules", json=payload) | ||
|
|
||
| def put(self, item_id: str, payload: json) -> Box: |
pyzscaler/zia/web_dlp.py
Outdated
| """ | ||
| return self._put(f"webDlpRules/{item_id}", json=payload) | ||
|
|
||
| def delete(self, item_id: str) -> requests.Response: |
pyzscaler/zia/web_dlp.py
Outdated
| Returns: | ||
| Requests.Response object. | ||
| """ | ||
| return self._delete(f"webDlpRules/{item_id}") |
There was a problem hiding this comment.
Often I've found ZIA responses to DELETE to be inconsistent. I've opted to just return the status code instead, which gives us at least a uniform response to expect on a successful DELETE.
I'd recommend:
return self._delete(f"webDlpRules/{item_id}").status_code
pyzscaler/zia/web_dlp.py
Outdated
| """ | ||
| return self._put(f"webDlpRules/{item_id}", json=payload) | ||
|
|
||
| def delete(self, item_id: str) -> requests.Response: |
There was a problem hiding this comment.
If we return the status code, just a reminder that the return type is Int.
|
SonarCloud Quality Gate failed.
|
|
I appreciate your detailed response. Let me try to give context to your notes:
|
|
Hey @daxm I've re-pointed this pull request to the feature branch so I'll approve the pull request and just submit another one for your latest changes and we can go from there. 👍 |
Updated my "main" with Mitch's. Also merged in my web_dlp branch.