aai-institute / lakefs-spec

An fsspec implementation for the lakeFS project.
http://lakefs-spec.org/
Apache License 2.0
37 stars 4 forks source link

Add lakeFS server version guard to `LakeFSFilesystem.touch()` #214

Closed nicholasjng closed 6 months ago

nicholasjng commented 6 months ago

Guards against attempted touch() operations by checking the backend version against the minimum supported backend version. This is preferable to the rather cryptic missing mime type 500 error from the backend in lakeFS server versions before v1.3.1.

Also adds a test mocking the server config to return an unsupported version. Skipping on old lakeFS server versions is currently not implemented, but lower prio, since the CI runs on the latest Docker image.

Fixes #208.

codecov[bot] commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (ad8437a) 89.80% compared to head (6a713ca) 90.01%.

:exclamation: Current head 6a713ca differs from pull request most recent head b5d3a26. Consider uploading reports for the commit b5d3a26 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #214 +/- ## ========================================== + Coverage 89.80% 90.01% +0.21% ========================================== Files 7 7 Lines 520 531 +11 Branches 82 85 +3 ========================================== + Hits 467 478 +11 Misses 27 27 Partials 26 26 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

AdrianoKF commented 6 months ago

Skipping on old lakeFS server versions is currently not implemented, but lower prio, since the CI runs on the latest Docker image.

@pytest.mark.xfail can take a boolean argument condition, which we could use to mark the test case as an expected failure in case of server versions <1.3.1.

nicholasjng commented 6 months ago

Skipping on old lakeFS server versions is currently not implemented, but lower prio, since the CI runs on the latest Docker image.

@pytest.mark.xfail can take a boolean argument condition, which we could use to mark the test case as an expected failure in case of server versions <1.3.1.

Do you know of a way of skipping a test based on a fixture value? I was looking through the docs and found nothing. My idea is this:

@pytest.skipif(lakefs_version < (1, 3, 1), reason="fs.touch() is not supported on old lakeFS server versions")
def test_touch():
    ...
AdrianoKF commented 6 months ago

Skipping on old lakeFS server versions is currently not implemented, but lower prio, since the CI runs on the latest Docker image.

@pytest.mark.xfail can take a boolean argument condition, which we could use to mark the test case as an expected failure in case of server versions <1.3.1.

Do you know of a way of skipping a test based on a fixture value? I was looking through the docs and found nothing. My idea is this:

@pytest.skipif(lakefs_version < (1, 3, 1), reason="fs.touch() is not supported on old lakeFS server versions")
def test_touch():
    ...

You could also call pytest.xfail() (I'd argue for xfail(raises=...) over skipif, so we get to validate the correct exception is thrown) inside the test case, where you have access to the fixture value. I also couldn't find any other solution from a quick web search.