-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
zendesk_ticket: new module #8543
base: main
Are you sure you want to change the base?
zendesk_ticket: new module #8543
Conversation
plugins/modules/zendesk_ticket.py
Outdated
ticket_id = module.params['ticket_id'] | ||
|
||
if not password and not token: | ||
module.fail_json(msg="Either 'password' or 'token' must be provided.") |
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.
Check out required_one_of
in https://docs.ansible.com/ansible/latest/dev_guide/developing_program_flow_modules.html#argument-spec-dependencies.
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.
Since you are using required_one_of
, this if
block here becomes redundant. Just remove it.
plugins/modules/zendesk_ticket.py
Outdated
elif status in ['closed', 'resolved']: | ||
if not ticket_id: | ||
module.fail_json(msg="The 'ticket_id' must be provided when the status is 'closed'") | ||
result = zendesk_api.close_ticket(ticket_id, status, body) |
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 don't see any idempotence checks here, like "is the ticket already closed" before closing it, or "does the ticket already exist" before creating it.
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.
@felixfontein comment still stands - it is not resolved. I believe it could be solved somewhat easily by adding the state as a parameter to check_ticket
and verifying whether the ticket state is different.
Also please check out the requirements in https://github.com/ansible-collections/community.general/blob/main/CONTRIBUTING.md#creating-new-modules-or-plugins for new modules. You need to add tests, for example. |
Co-authored-by: Felix Fontein <[email protected]>
Co-authored-by: Felix Fontein <[email protected]>
Co-authored-by: Felix Fontein <[email protected]>
…s for requirements
Added the failing mock code, here is the issue i am seeing on my end:
|
Co-authored-by: Felix Fontein <[email protected]>
Co-authored-by: Felix Fontein <[email protected]>
Co-authored-by: Felix Fontein <[email protected]>
This comment was marked as outdated.
This comment was marked as outdated.
Removed the http mock library, let me know if there are any other improvements or changes i can make to it. |
request = self.api_auth() | ||
try: | ||
response = request.get(url) | ||
except Exception: | ||
return False |
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.
This code is not wrong, however, for the user of this module, information is lost: why has it failed? what was the error message? They will have an error and have little idea about what happened.
This could be an improvement for another PR, no need to hold this one back over this.
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.
This test only proves that the code handles exceptions, it does not test how the code actually works: that was the idea of mocking.
Since the requests
library is not being used (to warrant the use of httmock
), maybe the answer is a direct mocking of the ansible.module_utils.urls.Request
class, or at the very least, of its methods .get()
, .patch()
, etc...
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.
Hmm, I guess we need a better mechanism for this. For open_url
and fetch_url
in the same module there are some unit test frameworks in https://github.com/ansible-collections/community.internal_test_tools/tree/main/tests/unit/utils, but not (yet) for the Request
class...
By the way, sorry for indicating httmock before, I obviously did not know it was specific to |
@russoz does the module look OK enough for you to merge for 9.4.0? |
I think it needs a bit polishing yet, specially in the tests. |
SUMMARY
Creating a new module to interface with the Zendesk ticketing system. With this module you are able to utilize the API from Zendesk to Create, Close and Resolve tickets.
ISSUE TYPE
COMPONENT NAME
zendesk_ticket.py