Add Hooks management API #227
Conversation
There was a problem hiding this comment.
Thanks! In general looks good. I left 2 comments. There are properties not publicly documented as available. Unless those are in the API swagger docs, we shouldn't add them to the SDK.
If anything, we can always add them later. But removing them later would be a breaking change.
auth0/v3/management/hooks.py
Outdated
| """ | ||
| return self.client.post(self._url(), data=body) | ||
|
|
||
| def get(self, id, fields=None, include_fields=True): |
There was a problem hiding this comment.
I don't see any property other than id documented here https://auth0.com/docs/api/management/v2#!/Hooks/get_hooks_by_id.
Please, remove fields and include_fields.
There was a problem hiding this comment.
Thanks for the review. Those fields are indeed not part of the documented fields but the descriptions states "Accepts a list of fields to include or exclude in the result." That's why I included them (I didn't test yet if they work or not).
I can remove them, but I also suggest to remove this comment from the documentation.
There was a problem hiding this comment.
I tested, and those fields and include_fieldsactually work for this API. So I would suggest fixing the doc (if that might take too long, I can remove them from the PR so they can be added later, just let me know).
There was a problem hiding this comment.
I could do the PR to fix the docs but I'd first like to check with the Hooks team why these properties are not documented (there might be a reason).
We can hold off merging this PR until that response comes back (might take ~10 days), or if you want, you can remove these properties for now and if the team later says they are valid, we can update the docs and this SDK on a separate PR.
How does that sound @guillp?
There was a problem hiding this comment.
No worries, I don't mind waiting for a few days until you sort it out internally.
There was a problem hiding this comment.
@guillp I got confirmation that for GET Hooks/{id} the fields argument is supported. What they didn't specify is about the include_fields argument. I assume it is, since both relate. Can you run a quick request and test that for me, please?
For secrets, there's no support for fields. So good that you removed them. I think we can get this merged and released today if you're around. Please, do rebase the branch since I merged a few PRs a few hours ago.
There was a problem hiding this comment.
Update from the team: include_fields is not supported on neither of these endpoints. Let's keep the fields arg only for this endpoint.
Regarding the tests, I retried the build and it passed 👍
|
not sure why that test fails, it is unrelated to the change I made. |
Changes
Adds the Hooks management API that was missing in auth0-python, along with unit tests.
It also includes Hooks Secrets management.
References
API docs: https://auth0.com/docs/api/management/v2#!/Hooks/get_hooks
Closes #224
Testing
Checklist