Drop pull_digest from hermetic boostrap image#1716
Drop pull_digest from hermetic boostrap image#1716tkopecek wants to merge 1 commit intorpm-software-management:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request correctly removes the pull_digest field from the metadata query for hermetic bootstrap images. This change addresses a potential issue where Digest and RepoDigests can mismatch, causing podman load to fail. The change is well-justified and improves the robustness of the hermetic build process. I have added one suggestion to improve the readability of a related log message, also aligning with preferred logging practices.
| get_query = '{"id": "{{.Id}}", "architecture": "{{ .Architecture }}"}' | ||
| getLog().info("Reading image %s from %s", get_query, self.image) | ||
| cmd = ["podman", "image", "inspect", "--format", get_query, self.image] |
There was a problem hiding this comment.
The log message on line 255 is confusing because it includes the raw query format string, making the log output difficult to interpret. Since get_query is only used once after this change, you can improve readability by inlining it into the podman command and writing a more descriptive log message. Additionally, prefer lazy '%s' formatting for log entries, as demonstrated in the suggestion, over f-strings or direct string concatenation for better performance and consistency. This aligns with the style guide's emphasis on readability.
| get_query = '{"id": "{{.Id}}", "architecture": "{{ .Architecture }}"}' | |
| getLog().info("Reading image %s from %s", get_query, self.image) | |
| cmd = ["podman", "image", "inspect", "--format", get_query, self.image] | |
| getLog().info("Reading image metadata from %s", self.image) |
References
- The style guide states in line 4: 'Readability: Code should be easy to understand for all team members.' The current log message is not easily understandable and thus violates this principle. (link)
- Prefer lazy '%s' formatting for log entries over f-strings.
|
copy paste from testing farm: |
6851b5e to
6ef9b9a
Compare
Image has one Digest and many RepoDigests. So, we can hit situation when these two values don't match and podman will refuse to load such tarball later with error like described here: containers/podman#27323 We should move to different mechanism (porbably skopeo) in the near future.
6ef9b9a to
471c830
Compare
|
Could be replaced by #1717 |
|
Yeah, let's go with #1717 |
Image has one Digest and many RepoDigests. So, we can hit situation when these two values don't match and podman will refuse to load such tarball later with error like described here:
containers/podman#27323
We should move to different mechanism (porbably skopeo) in the near future.