-
Notifications
You must be signed in to change notification settings - Fork 10
sap_control_exec - add local socket support #66
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
sap_control_exec - add local socket support #66
Conversation
Signed-off-by: Nicolas Bettembourg <[email protected]>
Signed-off-by: Nicolas Bettembourg <[email protected]>
Fix Python lint issues (mostly trailing whitespaces) Fix incorrect local connection class declaration when SUDS wasn't present * Signed-off-by: Nicolas Bettembourg <[email protected]>
* Fix local socket not handling do_open correctly Signed-off-by: Nicolas Bettembourg <[email protected]>
|
Sorry for all these commits, if squashing is allowed it might be cleaner to do so. Current version has been tested on my side and should be OK with CI. |
|
@nbttmbrg Squash is not the best, but some of your changes broke tests so if you changed structure of python module, you have to update sanity test as well. When I do troubleshooting like this, I will just re-create PR afresh to get rid of debugging commits. CI Workflow in |
Signed-off-by: Nicolas Bettembourg <[email protected]>
…/community.sap_libs into sapcontrol-local-socket Signed-off-by: Nicolas Bettembourg <[email protected]>
* Sanity: Fix single new line between functions Signed-off-by: Nicolas Bettembourg <[email protected]>
|
@nbttmbrg You can configure your environment to use |
Signed-off-by: Nicolas Bettembourg <[email protected]>
Signed-off-by: Nicolas Bettembourg <[email protected]>
|
It finally looks ok ! I'm used to develop locally and we are currently not testing things thoroughly enough, it seems... I learnt a lot with all this, so thank you very much for your help 👍 I would still appreciate a human to have a look at the code of the module and its test though ! |
|
@nbttmbrg That is good that it was learning experience. You can bet on that I will not randomly approve it so give me few days to get time to review it and test it out. |
marcelmamula
left a comment
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. I have tested this change and it works on SLES 16.
Without change: fatal: [localhost]: FAILED! => {"changed": false, "error": "(401, 'Unauthorized')", "msg": "Something went wrong connecting to the SAPCONTROL SOAP API.", "out": {}}
With change: Function GetVersionInfo correctly works.
Thank you @nbttmbrg . Can you check if you can merge this PR yourself (Merge with Merge Commit, not squash)?
Sadly I don't have the necessary permissions to Merge on this repo. |
|
@marcelmamula @nbttmbrg I see quite some similarities in some classes and variables names (for instance): If so, I encourage you to correctly reflect this in the license of this repo, because mentioned code is GPL3.0 not Apache2. Thank you |
It was in fact influenced by this repo, I wanted to reproduce the same behavior so that's why some parts of the code are similar. I will let @marcelmamula edit the license file (or delete these changes if it's somehow better). Edit : And I apologize for this, I didn't meant no harm but I understand that re-using parts of code shouldn't be done. Lesson learned. |
|
@nbttmbrg We will not be adjusting any license files because having more than one license in repo will create much more issues. @kksat does not want to give permission to use this code to this project outside of GPL-3 from what I heard. You can look at that code and think how to achieve same result, but differently like not using classes, but instantiating inside of functions, etc. If you dont have any solution how to replace, we can do revert to previous commit and remove this PR from |
|
I will work on this and make another PR. Thanks to both of you. |
|
Changes can remain in dev as they are not merged to main yet, and you can branch of current dev. Socket check functionality requires I will not be available next week so I will not be able to respond. |
|
If there is no other solution, then we can discuss making this module (and only this module) as GPL-3 or reverting back. |
|
@kksat The following is speaking personally, as I no longer represent any company, or have any professional allegiances, or have any direct involvements to these projects - I am an observer with personal pride in this little segment of the wider open-source community. Firstly, stop stalking the PR contributions and creating combative commentary that does not provide value to the community. This is not the first time various members of the community or your own leadership have had discussions with you about this, including but not limited to - this is not the first time you have parachuted into GitHub Issues/PRs to start throwing grenades without any contributions (or even testing the code). We have a welcoming community-first space, where competing companies work together to create something that has a meaningful impact to other companies in this space. That's rare to have such harmony, and it is to be protected. As you've repeatedly demonstrated over many years, you do not act in accordance with the values and spirit of this initiative. Within your company, others contribute to the community and you act in isolation. Plainly speaking, enough is enough - I wish you the best in your professional career and code projects, but I ask that you please leave this community and never return. Secondly, the SLL-OSI has always been community-first with code governed under the permissive Apache licence so as to allow derivative works by other vendors without Intellectual Property (IP) concerns. This is explicitly stated from the beginning in all internal documents prior to SLL-OSI launch. Bluntly speaking the permissive licence was in the business interest (of Red Hat, who monetised this code first via RHEL System Roles and RHT AAP) by removing legal licence impediments/restrictions so that adoption would be greater across customers/partners who would consume the code, extend it, sell their own product/service based on it - and hopefully sometimes contribute bug fixes etc back to the community. This approached allowed for extension with their own IP, based upon a common foundation provided by the community. A win-win scenario for everyone involved, where the code could be sold with support + extended with IP. Your failed attempts sap-linuxlab/community.sap_operations#12 and sap-linuxlab/community.sap_operations#14 to dump code into the community space (for community members to maintain on your behalf) without integrating/cleaning your code into pre-existing code was rude; especially given you maliciously used a duplicate name (what was wrong with Back to the matter at hand. When you created these PRs, it constituted acknowledgement that the code is gifted to the community by you under the initiative's and project's firm rule to use Apache licence. Your acceptance of this can be seen in your code contribution, where you amended licence files to Apache and metadata to Apache. The code contribution includes the aforementioned classes. In addition there was even an internal email about this various years ago, where the licence was supposed to be altered in the At various occasions, in the past few years, community members wanted to resolve this divide you created. We all thought about the effort to merge your (bluntly again, rogue) code to create a combined code line where there is genuinely valuable features you have, tied into features that exist under Plainly speaking once more, you already gave this code to the community in the PR - please drop your absurd complaint and never return. |
|
@nbttmbrg I am very sorry your first PR had to contain this historic ugliness. Our community is very grateful to have you and your colleagues, we thank you for your contribution and look forward to more in the future as your usage increases 🙂 |
Add support for local socket connection in sap_control_exec when hostname is localhost and sysnr is provided without username/password