Conversation
When building with older buildah, we have to use the `-v $PWD:/run/src` trick, but then we need to clean up the OCI archive. I don't like using the `RUN` trick to remove the OCI archive for various reasons. But it's trivial anyway to just clean it up ourselves.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a reproducibility issue by sorting extended attributes during the scan phase. The changes are logical and well-implemented. The introduction of trusted. xattr filtering is a sensible addition. The end-to-end tests have been significantly improved to validate reproducibility by building the same image twice and comparing their IDs, which is an excellent approach. The test setup is also more robust by using a pre-built rootfs image. Overall, this is a high-quality contribution.
Yet another source of reproducibility issues. If a file has multiple xattrs, the order in which we read them in may change, but serializing them to the tar layer needs to be deterministic. Sort them at scan time. Extend the buildtime e2e test to try to cover this by adding more xattrs (including ACLs which we didn't have any coverage for before). Though it's not easy to really test this without injecting our own llistxattr(2). Assisted-by: Claude Opus 4.6
Noticed this while working on the previous commit. We were picking up legacy XFS SGI_ACL_* xattr aliases for ACLs, which is just yucky. AFAICT there's no use case for preserving some trusted xattrs, so just skip them all for now until we find out otherwise. Assisted-by: Claude Opus 4.6
jlebon
force-pushed
the
trusted-filter
branch
from
84591f5 to
53fc7a3
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Yet another source of reproducibility issues. If a file has multiple
xattrs, the order in which we read them in may change, but serializing
them to the tar layer needs to be deterministic.
Sort them at scan time.
Extend the buildtime e2e test to try to cover this by adding more
xattrs (including ACLs which we didn't have any coverage for before).
Though it's not easy to really test this without injecting our own
llistxattr(2).
Assisted-by: Claude Opus 4.6