sap_control_exec, sap_hostctrl_exec: Refactor to use shared utilities#78
Conversation
- Consolidate SOAP connection logic into module_utils/sapcontrol_soap.py - Update sap_control_exec.py and sap_hostctrl_exec.py to use shared utilities - Refactor and patch unit tests for correct mocking of shared Client Signed-off-by: Melvin Malagowski <mmalagowski@oxya.com>
|
@MelvinM-coder We already had other changes coming from Oxya team so it is welcomed to see new contributors. All tests have passed, but this PR needs some changes because it is not working. |
There was a problem hiding this comment.
@MelvinM-coder This is currently non-working PR and it removed functionalities that existed.
Please go back to drawing board and properly recreate existing functionality. There are lot of things that are not required like call_sap_control which can be simple connection call with service name parameter.
I will not be testing any further because currently this is non-working.
Example of simple test I used, that fails instantly, but works on current code in main:
- name: Ansible Play to test sap_libs modules
hosts: bw
gather_facts: false
tasks:
- name: GetProcessList - SOCKET
community.sap_libs.sap_control_exec:
sysnr: '01'
function: GetProcessListSigned-off-by: Yannick Douvry <ydouvry@oxya.com>
Signed-off-by: Yannick Douvry <ydouvry@oxya.com>
marcelmamula
left a comment
There was a problem hiding this comment.
@MelvinM-coder @ydouvry I have tested last changes and it is working now, but there are still 2 unanswered comments/issues so there is no point in reviewing now.
|
@MelvinM-coder I will be off next week and since there were lot of issues in first version of this PR, I will reserve right to do proper testing once I am back. |
…sapstartsrv_client.py Signed-off-by: Melvin Malagowski<mmalagowski@oxya.com>
Signed-off-by: Melvin Malagowski <mmalagowski@oxya.com>
|
Hello @marcelmamula |
marcelmamula
left a comment
There was a problem hiding this comment.
@MelvinM-coder @ydouvry LGTM
I have tested these changes and I can confirm it works and looks better after we resolved all open issues.
Let me know if you dont have access to merge this PR (with Merge Commit) and I will do it.
FYI: There are 8+ items in my TODO list to fix with these modules, that I identified during my testing so keep that in mind before you start preparing changes you mentioned.
This is my first public pull request. Feedback is welcome, and I’m happy to adjust anything as needed. ^^
I did this refactor mainly to avoid duplicating the SOAP connection logic and to make it reusable across multiple modules. This is also the first step toward a follow-up
system_statemodule to manage/report system and instance state consistently, which I plan to open in a next PR.Signed-off-by: Melvin Malagowski mmalagowski@oxya.com