fix: preserve xattrs in layers#1016
Conversation
upils
left a comment
upils
left a comment
There was a problem hiding this comment.
I agree that adding a spread test would be very welcomed because with the current tests I am very unsure of what this implementation does.
Maybe the unit tests can be expanded to check the behavior of the added for loop (since it does not depend on the call to tar).
| if not temp_tar_file.parent.exists(): | ||
| temp_tar_file.parent.mkdir(parents=True) |
There was a problem hiding this comment.
question: was it done transparently by tarfile.open() before? I expected the caller of archive_layer() to make sure the directory holding temp_tar_file existed.
There was a problem hiding this comment.
I believe the previous code assumed the directory existed; we can preserve that assumption and perhaps document it
There was a problem hiding this comment.
Interesting. This was one of the first lines I added and it did fix things, but it looks like it isn't needed anymore. It's removed now
| for arcname in sorted(layer_paths, reverse=True): | ||
| filepath = layer_paths[arcname] | ||
| emit.debug(f"Adding to layer: {filepath} as '{arcname}'") | ||
| tar_file.add(filepath, arcname=arcname, recursive=False) | ||
| emit.debug(f"Adding to layer: {filepath} as {arcname!r}") | ||
|
|
||
| # Construct a new file at `arcname` with the same contents as `filepath`. | ||
| # This emulates the `arcname` parameter of `tarfile.open()`, which is not | ||
| # present in GNU tar. | ||
| new_path = tmppath / arcname | ||
| layer_contents.append(new_path.relative_to(tmppath)) | ||
| if new_path.is_dir() and new_path.exists(): | ||
| continue | ||
| new_path.parent.mkdir(parents=True, exist_ok=True) | ||
| filepath.rename(new_path) |
There was a problem hiding this comment.
This whole piece is confusing me. Despite the comment I am very unsure what it is doing. By recreating directories are we not risking losing permissions/attributes on them?
| if not temp_tar_file.parent.exists(): | ||
| temp_tar_file.parent.mkdir(parents=True) |
There was a problem hiding this comment.
I believe the previous code assumed the directory existed; we can preserve that assumption and perhaps document it
| for arcname in sorted(layer_paths, reverse=True): | ||
| filepath = layer_paths[arcname] | ||
| emit.debug(f"Adding to layer: {filepath} as '{arcname}'") | ||
| tar_file.add(filepath, arcname=arcname, recursive=False) | ||
| emit.debug(f"Adding to layer: {filepath} as {arcname!r}") | ||
|
|
||
| # Construct a new file at `arcname` with the same contents as `filepath`. | ||
| # This emulates the `arcname` parameter of `tarfile.open()`, which is not | ||
| # present in GNU tar. | ||
| new_path = tmppath / arcname | ||
| layer_contents.append(new_path.relative_to(tmppath)) | ||
| if new_path.is_dir() and new_path.exists(): | ||
| continue | ||
| new_path.parent.mkdir(parents=True, exist_ok=True) | ||
| filepath.rename(new_path) |
|
I think we should try this AI suggestion next |
3 participants
make lint && make test?Prototype implementation of xattr preservation using GNU tar in Rockcraft. The fix was tested locally using the reproducer in the linked issue. If reviewers are happy with the final implementation, I'll add a spread test confirming this behavior.
Closes #683.
ROCKCRAFT-282