bolkedebruin commented on code in PR #37524:
URL: https://github.com/apache/airflow/pull/37524#discussion_r1494298461
##########
tests/io/test_path.py:
##########
@@ -60,34 +89,39 @@ def get_fs_no_storage_options(_: str):
class TestFs:
def setup_class(self):
self._store_cache = _STORE_CACHE.copy()
+ self._fsspec_registry = _fsspec_registry.copy()
+ for protocol in FakeRemoteFileSystem.protocol:
+ register_implementation(protocol, FakeRemoteFileSystem,
clobber=True)
def teardown(self):
_STORE_CACHE.clear()
_STORE_CACHE.update(self._store_cache)
+ _fsspec_registry.clear()
+ _fsspec_registry.update(self._fsspec_registry)
def test_alias(self):
store = attach("file", alias="local")
assert isinstance(store.fs, LocalFileSystem)
assert "local" in _STORE_CACHE
def test_init_objectstoragepath(self):
- path = ObjectStoragePath("file://bucket/key/part1/part2")
- assert path.bucket == "bucket"
+ path = ObjectStoragePath("file:///key/part1/part2")
+ assert path.bucket == ""
Review Comment:
I agree the current defined behavior is:
* bucket = netloc
* a url is {protocol}://{bucket}/{key} # note the hardcoded /
* path = bucket / key # again note the '/' is hardcoded
##########
tests/io/test_path.py:
##########
@@ -227,16 +271,13 @@ def test_move_remote(self):
_to.unlink()
def test_copy_remote_remote(self):
- # foo = xxx added to prevent same fs token
- attach("ffs", fs=FakeRemoteFileSystem(auto_mkdir=True, foo="bar"))
- attach("ffs2", fs=FakeRemoteFileSystem(auto_mkdir=True, foo="baz"))
+ attach("ffs", fs=FakeRemoteFileSystem(skip_instance_cache=True))
+ attach("ffs2", fs=FakeRemoteFileSystem(skip_instance_cache=True))
- dir_src = f"/tmp/{str(uuid.uuid4())}"
- dir_dst = f"/tmp/{str(uuid.uuid4())}"
+ dir_src = f"bucket1/{str(uuid.uuid4())}"
Review Comment:
I think it would be good to test that we have a `path.bucket` here.
##########
airflow/providers/common/io/xcom/backend.py:
##########
@@ -152,7 +152,7 @@ def deserialize_value(
try:
p = ObjectStoragePath(path) / XComObjectStoreBackend._get_key(data)
- return json.load(p.open("rb", compression="infer"),
cls=XComDecoder)
+ return json.load(p.fs.open(p.path, mode="rb",
compression="infer"), cls=XComDecoder)
Review Comment:
I agree. Any reason to use `fs.open` directly here @ap-- ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
