Address PR comments

man-group · May 17, 2024 · cca0889 · cca0889
1 parent 7ebe7a3
commit cca0889
Show file tree

Hide file tree

Showing 5 changed files with 11 additions and 9 deletions.
diff --git a/docs/mkdocs/docs/api/arctic_uri.md b/docs/mkdocs/docs/api/arctic_uri.md
@@ -31,7 +31,7 @@ specifies the region and `region` is not set.
 
 The Azure URI connection scheme has the form `azure://[options]`.
 It is based on the Azure Connection String, with additional options for configuring ArcticDB.
-Please refer to https://learn.microsoft.com/en-us/azure/storage/common/storage-configure-connection-string for more details.
+Please refer to [Azure](https://learn.microsoft.com/en-us/azure/storage/common/storage-configure-connection-string) for more details.
 
 `options` is a string that specifies connection specific options as `<name>=<value>` pairs joined with `;` (the final key value pair should not include a trailing `;`).
 
@@ -42,17 +42,17 @@ Additional options specific for ArcticDB:
 | Container     | Azure container for blobs |
 | Path_prefix   | Path within Azure container to use for data storage |
 | CA_cert_path  | (Linux platform only) Azure CA certificate path. If not set, python ``ssl.get_default_verify_paths().cafile`` will be used. If the certificate cannot be found in the provided path, an Azure exception with no meaningful error code will be thrown. For more details, please see [here](https://github.com/Azure/azure-sdk-for-cpp/issues/4738). For example, `Failed to iterate azure blobs 'C' 0:`.|
-| CA_cert_dir   | (Linux platform only) Azure CA certificate directory. If not set, python ``ssl.get_default_verify_paths().capath`` will be used. Certificates can only be used if corresponding hash files exist (https://www.openssl.org/docs/man1.0.2/man3/SSL_CTX_load_verify_locations.html). If the certificate cannot be found in the provided path, an Azure exception with no meaningful error code will be thrown. For more details, please see [here](https://github.com/Azure/azure-sdk-for-cpp/issues/4738). For example, `Failed to iterate azure blobs 'C' 0:`.|
+| CA_cert_dir   | (Linux platform only) Azure CA certificate directory. If not set, python ``ssl.get_default_verify_paths().capath`` will be used. Certificates can only be used if corresponding hash files [exist](https://www.openssl.org/docs/man1.0.2/man3/SSL_CTX_load_verify_locations.html). If the certificate cannot be found in the provided path, an Azure exception with no meaningful error code will be thrown. For more details, please see [here](https://github.com/Azure/azure-sdk-for-cpp/issues/4738). For example, `Failed to iterate azure blobs 'C' 0:`.|
 
-For non-Linux platform, `CA_cert_path` AND `CA_cert_dir` cannot be set. Please set CA certificate related option on corresponding OS setting.
-For Windows, you may refer to https://learn.microsoft.com/en-us/skype-sdk/sdn/articles/installing-the-trusted-root-certificate
+For non-Linux platform, `CA_cert_path` AND `CA_cert_dir` cannot be set. Please set CA certificate related option on corresponding setting in OS.
+For Windows, please see [here](https://learn.microsoft.com/en-us/skype-sdk/sdn/articles/installing-the-trusted-root-certificate)
 
 Exception: Azure exceptions message always ends with `{AZURE_SDK_HTTP_STATUS_CODE}:{AZURE_SDK_REASON_PHRASE}`.
 
-Please refer to https://github.com/Azure/azure-sdk-for-cpp/blob/24ed290815d8f9dbcd758a60fdc5b6b9205f74e0/sdk/core/azure-core/inc/azure/core/http/http_status_code.hpp for
+Please refer to [azure-sdk-for-cpp](https://github.com/Azure/azure-sdk-for-cpp/blob/24ed290815d8f9dbcd758a60fdc5b6b9205f74e0/sdk/core/azure-core/inc/azure/core/http/http_status_code.hpp) for
 more details of provided status codes.
 
-Note that due to a bug in Azure C++ SDK (https://github.com/Azure/azure-sdk-for-cpp/issues/4738), Azure may not give meaningful status codes and
+Note that due to a [bug](https://github.com/Azure/azure-sdk-for-cpp/issues/4738) in Azure C++ SDK, Azure may not give meaningful status codes and
 reason phrases in the exception. To debug these instances, please set the environment variable `export AZURE_LOG_LEVEL` to `1` to turn on the SDK debug logging.
 
 

diff --git a/python/arcticdb/adapters/azure_library_adapter.py b/python/arcticdb/adapters/azure_library_adapter.py
@@ -27,7 +27,7 @@
 @dataclass
 class ParsedQuery:
     Path_prefix: Optional[str] = None
-    # winhttp is used as Azure backend support on Windows by default; winhttp itself mainatains ca cert.
+    # winhttp is used as Azure backend support on Windows by default; winhttp itself maintains ca cert.
     # The options should be left empty else libcurl will be used on Windows
     CA_cert_path: Optional[str] = "" # CURLOPT_CAINFO in curl
     CA_cert_dir: Optional[str] = "" # CURLOPT_CAPATH in curl

diff --git a/python/arcticdb/adapters/s3_library_adapter.py b/python/arcticdb/adapters/s3_library_adapter.py
@@ -47,7 +47,7 @@ class ParsedQuery:
     # DEPRECATED - see https://github.com/man-group/ArcticDB/pull/833
     force_uri_lib_config: Optional[bool] = True
 
-    # winhttp is used as s3 backend support on Windows by default; winhttp itself mainatains ca cert.
+    # winhttp is used as s3 backend support on Windows by default; winhttp itself maintains ca cert.
     # The options has no effect on Windows
     CA_cert_path: Optional[str] = "" # CURLOPT_CAINFO in curl
     CA_cert_dir: Optional[str] = "" # CURLOPT_CAPATH in curl

diff --git a/python/tests/conftest.py b/python/tests/conftest.py
@@ -111,6 +111,7 @@ def lmdb_library(lmdb_storage, lib_name):
     return lmdb_storage.create_arctic().create_library(lib_name)
 
 
+# ssl is enabled by default to maximize test coverage as ssl is enabled most of the times in real world
 @pytest.fixture(scope="session")
 def s3_storage_factory():
     with MotoS3StorageFixtureFactory(use_ssl=SSL_TEST_ENABLED) as f:
@@ -166,7 +167,7 @@ def real_s3_storage(real_s3_storage_factory):
     with real_s3_storage_factory.create_fixture() as f:
         yield f
 
-
+# ssl cannot be ON by default due to azurite performance constraints https://github.com/man-group/ArcticDB/issues/1539
 @pytest.fixture(scope="session")
 def azurite_storage_factory():
     with AzuriteStorageFixtureFactory(use_ssl=False) as f:

diff --git a/python/tests/integration/arcticdb/test_arctic.py b/python/tests/integration/arcticdb/test_arctic.py
@@ -44,6 +44,7 @@ class ParameterDisplayStatus(Enum):
 
 parameter_display_status = [ParameterDisplayStatus.NOT_SHOW, ParameterDisplayStatus.DISABLE, ParameterDisplayStatus.ENABLE]
 no_ssl_parameter_display_status = [ParameterDisplayStatus.NOT_SHOW, ParameterDisplayStatus.DISABLE]
+
 class DefaultSetting:
     def __init__(self, factory):
         self.cafile = factory.client_cert_file