-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Support the hdbcli
client library
#10595
Conversation
e12cb70
to
ee92184
Compare
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.
Looks good, I left a few comments
https://help.sap.com/viewer/f1b440ded6144a54ada97ff95dac7adf/2.10/en-US/ee592e89dcce4480a99571a4ae7a702f.html | ||
value: | ||
type: object | ||
additionalProperties: true |
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.
What does this mean?
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.
Allows for anything https://swagger.io/docs/specification/data-models/dictionaries/#free-form
if USING_HDBCLI: | ||
|
||
def get_connection(self): | ||
# https://help.sap.com/viewer/f1b440ded6144a54ada97ff95dac7adf/2.10/en-US/ee592e89dcce4480a99571a4ae7a702f.html | ||
connection_properties = self.instance.get('connection_properties', {}).copy() | ||
|
||
connection_properties.setdefault('address', self._server) | ||
connection_properties.setdefault('port', self._port) | ||
connection_properties.setdefault('user', self._username) | ||
connection_properties.setdefault('password', self._password) | ||
|
||
timeout_milliseconds = int(self._timeout * 1000) | ||
connection_properties.setdefault('communicationTimeout', timeout_milliseconds) | ||
connection_properties.setdefault('nodeConnectTimeout', timeout_milliseconds) | ||
|
||
if self._use_tls: | ||
connection_properties.setdefault('encrypt', True) | ||
connection_properties.setdefault('sslHostNameInCertificate', self._server) | ||
connection_properties.setdefault('sslSNIHostname', self._server) | ||
|
||
tls_verify = self.instance.get('tls_verify', True) | ||
if not tls_verify: | ||
connection_properties.setdefault('sslValidateCertificate', False) | ||
|
||
tls_cert = self.instance.get('tls_cert') | ||
if tls_cert: | ||
connection_properties.setdefault('sslKeyStore', tls_cert) | ||
|
||
tls_ca_cert = self.instance.get('tls_ca_cert') | ||
if tls_ca_cert: | ||
connection_properties.setdefault('sslTrustStore', tls_ca_cert) | ||
elif not connection_properties.get('sslUseDefaultTrustStore', True): | ||
connection_properties.setdefault('sslTrustStore', certifi.where()) | ||
|
||
try: | ||
connection = HanaConnection(**connection_properties) | ||
except Exception as e: | ||
error = str(e) | ||
self.log.error('Unable to connect to SAP HANA: %s', error) | ||
self.service_check(self.SERVICE_CHECK_CONNECT, self.CRITICAL, message=error, tags=self._tags) | ||
else: | ||
self.service_check(self.SERVICE_CHECK_CONNECT, self.OK, tags=self._tags) | ||
return connection | ||
|
||
else: | ||
|
||
def get_connection(self): | ||
try: | ||
tls_context = self.get_tls_context() if self._use_tls else None | ||
connection = HanaConnection( | ||
host=self._server, | ||
port=self._port, | ||
user=self._username, | ||
password=self._password, | ||
tls_context=tls_context, | ||
timeout=self._timeout, | ||
) | ||
connection.connect() | ||
except Exception as e: | ||
error = str(e) | ||
self.log.error('Unable to connect to SAP HANA: %s', error) | ||
self.service_check(self.SERVICE_CHECK_CONNECT, self.CRITICAL, message=error, tags=self._tags) | ||
else: | ||
self.service_check(self.SERVICE_CHECK_CONNECT, self.OK, tags=self._tags) | ||
return connection |
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.
Should this be in connection.py?
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.
No b/c I think it makes more sense for the split in config logic to be closer to the check since it needs attributes of the check
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.
Docs review
Co-authored-by: ruthnaebeck <19349244+ruthnaebeck@users.noreply.github.com>
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.
👍 for docs
Motivation
https://github.com/SAP-archive/PyHDB