Skip to content

Conversation

@akreuzer
Copy link

Main effort is to split Connection in sap/adt/core.py into ConnectionViaRFC and ConnectionViaHTTP.

I don't have many test for this as this would require a Netweaver Server.

Main effort is to split Connection in sap/adt/core.py into ConnectionViaRFC and ConnectionViaHTTP.
@jfilak
Copy link
Owner

jfilak commented Apr 13, 2022

Good job! I was always wondering how that RFC HTTP tunneling works :) I am happy to merge once we get rid of those unnecessary style fixes and all tests and linters are satisfied.

Revert changes to formating
Revert to old requirements.txt and move urllib3 back
@akreuzer
Copy link
Author

Thank you. I must have accidentally run yapf on the changes.
I should have reverted all of the cosmetic changes and also the stuff with urllib3.

help="SAP Secure Login Client library (e.g. "
"/Applications/Secure Login Client.app/Contents/MacOS/lib/libsapcrypto.dylib")

arg_parser.add_argument("--rest-over-rfc", action='store_true', dest="rest_over_rfc",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay, I didn't notice earlier. The generic name 'rest-over-rfc' suggest that all HTTP traffic will be routed via RFC but I do believe only ADT HTTP traffic can be tunneled via RFC. Am I right?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a very interesting question. In theory, all of the HTTP requests can be tunneled because it directly connects to the HTTP dispatcher on Netweaver Server. However, I did not try with other types of requests and it is not implemented.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so since the implementation does not route entire HTTP traffic via RFC, I suggest to rename to "adt_rest_over_rfc".

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a good point.
I think it is better to keep the name but show an error message. With this, the parameter would not have to change if this is also implemented for gtcs. This is ultimately your call.

@jfilak
Copy link
Owner

jfilak commented Apr 15, 2022

Please update doc/configuration.md with all the recent congiration options you have added.

@akreuzer
Copy link
Author

  • Added new options to doc/configuration.md
  • Fixed on typo in bin/sapcli.
  • Fixed some issue with liniting.
  • Added NotImplementedException to the case where --rest-over-rfc is used with odata/gtcs. (This is just to illustrated - if you don't like it I can revert this commit.)

- Fix sapcli test (rename `group` to `rfc_group`
- Fix type hints for python 3.8 (Dict[...] instead of dict[..])
@lucasborin
Copy link
Collaborator

lucasborin commented Apr 27, 2022

If you do not mind, I would like to test it. However, pulling your forked version raised a syntax error:
image

Besides, I added these two entries to my cfg file:

export SAPCLI_HTTP_TIMEOUT=2000
export SAP_REST_OVER_RFC=yes

Any clue?

Git stauts:

image

@akreuzer
Copy link
Author

I think I know what this is:
list[str] as a type definition is only allowed in Python 3.9+

For older version on has to use something like this

from typing import List
a: List[str]

@lucasborin I updated the code.
Let me know if it works now.

@lucasborin
Copy link
Collaborator

Unsure if I did something wrong, but my python is not able to find PyRFC anymore before pulling it. I'll try to redo my setup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants