-
Notifications
You must be signed in to change notification settings - Fork 2
#245-adding-backend-inference-for-the-path-api #246
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
base: main
Are you sure you want to change the base?
#245-adding-backend-inference-for-the-path-api #246
Conversation
test/integration/test_path.py
Outdated
assert _collect_all_names(poems_root) == expected_names | ||
|
||
|
||
def test_infer_path_onprem(backend_aware_bucketfs_params): |
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.
backend_aware_bucketfs_params returns SaaS or on-prem parameters, so if you specifically want to test on-prem, you need to skip SaaS. Alternatively, we could also think about, only having one test and check on the inferred path, if we can upload files to the path.
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.
I vote to keep two tests, as the checks in one test would become more complex.....
exasol/bucketfs/_path.py
Outdated
saas_database_id: str | None = None, | ||
saas_database_name: str | None = None, | ||
saas_token: str | None = None, | ||
) -> str: |
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.
It is probably better to return the StorageBackend enum than a string.
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.
Unfortunately, enum StorageBackend is part of notebook-connector, which BFSPY does not depend on (an should not!).
We could decide to implement another enum here, though.
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.
I meant the StorageBackend in this repo. It already exist.
Co-authored-by: Mikhail Beck <mikhail.beck@exasol.com>
|
bucketfs_user=backend_aware_bucketfs_params["username"], | ||
bucketfs_password=backend_aware_bucketfs_params["password"], | ||
path_in_bucket="onpremtest/", | ||
bucketfs_use_https=backend_aware_bucketfs_params["verify"], |
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.
This will fail if a backend other than onprem is selected.
Compare
- backend_aware_bucketfs_params
- backend_aware_saas_bucketfs_params
- backend_aware_onprem_bucketfs_params
all in pytest-plugins/pytest-backend
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.
OK - please excuse me.
I probably was wrong.
I didn't notice that you test case should only be executed onprem, but skipped on saas.
I now replaced fixture backend
by use_onprem
which should serve this purpose in my suggestion above.
Still, the test case needs to call pytest.skip()
by himself.
) | ||
|
||
|
||
class StorageBackend(Enum): |
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.
As @ahsimb said: This probably could go to production code.
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.
Or simply use StorageBackend, maybe making it public by importing in bucketfs/__init__.py
.
pat=saas_token, | ||
path=path_in_bucket, | ||
) | ||
else: |
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.
Sorry, for a late comment. I think it might be useful to also include the Mounted BucketFS path here (and in the infer_backend
). This backend is just a file path that can be used by either a UDF or for testing. See build_path
.
The ValueError would probably be "Unsupported backend". Insufficient parameters would be thrown by the infer_backend
. If it was happy with the parameters, this must be some kind of a new backend, this function is unaware of or really doesn't want to support. This is a very small thing of course.
saas_database_id="dbid", | ||
saas_token="token", | ||
) | ||
assert result == "saas" |
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.
Compare to the enum
Co-authored-by: Christoph Kuhnke <github@kuhnke.net>
Co-authored-by: Christoph Kuhnke <github@kuhnke.net>
Co-authored-by: Mikhail Beck <mikhail.beck@exasol.com>
Co-authored-by: Mikhail Beck <mikhail.beck@exasol.com>
test/integration/test_path.py
Outdated
def test_infer_path_onprem(backend, backend_aware_bucketfs_params): | ||
""" | ||
Creates the PathLike and validates it. | ||
""" | ||
if backend == "saas": | ||
pytest.skip() |
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.
def test_infer_path_onprem(backend, backend_aware_bucketfs_params): | |
""" | |
Creates the PathLike and validates it. | |
""" | |
if backend == "saas": | |
pytest.skip() | |
def test_infer_path_onprem(backend_aware_bucketfs_params, use_onprem): | |
""" | |
Creates the PathLike and validates it. | |
""" | |
if not use_onprem: | |
pytest.skip() |
bucketfs_user=backend_aware_bucketfs_params["username"], | ||
bucketfs_password=backend_aware_bucketfs_params["password"], | ||
path_in_bucket="onpremtest/", | ||
bucketfs_use_https=backend_aware_bucketfs_params["verify"], |
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.
OK - please excuse me.
I probably was wrong.
I didn't notice that you test case should only be executed onprem, but skipped on saas.
I now replaced fixture backend
by use_onprem
which should serve this purpose in my suggestion above.
Still, the test case needs to call pytest.skip()
by himself.
test/integration/test_path.py
Outdated
def test_infer_path_saas( | ||
backend, saas_host, saas_pat, saas_account_id, backend_aware_saas_database_id | ||
): | ||
""" | ||
Creates the SaasBucket with fixture details realted to Saas and validates it. | ||
""" | ||
if backend != "saas": | ||
pytest.skip("The test runs only with SaaS database") |
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.
def test_infer_path_saas( | |
backend, saas_host, saas_pat, saas_account_id, backend_aware_saas_database_id | |
): | |
""" | |
Creates the SaasBucket with fixture details realted to Saas and validates it. | |
""" | |
if backend != "saas": | |
pytest.skip("The test runs only with SaaS database") | |
def test_infer_path_saas( | |
use_saas, saas_host, saas_pat, saas_account_id, backend_aware_saas_database_id | |
): | |
""" | |
Creates the SaasBucket with fixture details realted to Saas and validates it. | |
""" | |
if not use_saas: | |
pytest.skip("The test runs only with SaaS database") |
Co-authored-by: Christoph Kuhnke <github@kuhnke.net>
Co-authored-by: Christoph Kuhnke <github@kuhnke.net>
Closes #245