-
Notifications
You must be signed in to change notification settings - Fork 4
Blog: Add "Containers: pitfalls of incomplete tar archives" #20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request adds a new, detailed blog post exploring the nuances of tar archives within container image layers, the current limitations in the OCI specification, and the role of overlayfs. The post is well-written and provides a deep dive into a complex topic. My review includes several suggestions to fix minor typos, correct a link, and resolve a technical inconsistency to enhance the post's overall quality and accuracy.
content/blog/2025-dec-15-blog-containers-pitfalls-of-incomplete-tar-archives.md
Show resolved
Hide resolved
content/news/2025-dec-10-blog-containers-pitfalls-of-incomplete-tar-archives.md
Outdated
Show resolved
Hide resolved
content/news/2025-dec-10-blog-containers-pitfalls-of-incomplete-tar-archives.md
Outdated
Show resolved
Hide resolved
content/news/2025-dec-10-blog-containers-pitfalls-of-incomplete-tar-archives.md
Outdated
Show resolved
Hide resolved
content/news/2025-dec-10-blog-containers-pitfalls-of-incomplete-tar-archives.md
Outdated
Show resolved
Hide resolved
content/news/2025-dec-10-blog-containers-pitfalls-of-incomplete-tar-archives.md
Outdated
Show resolved
Hide resolved
content/news/2025-dec-10-blog-containers-pitfalls-of-incomplete-tar-archives.md
Outdated
Show resolved
Hide resolved
|
A few more outstanding things in my mind before we publish:
|
|
Also not sure why CI is failing with: Those are definitely valid anchors on that page 🤷 |
Pretty sure this is because the raw page content doesn't contain those anchors but they exist inside of a big |
Signed-off-by: John Eckersberg <jeckersb@redhat.com>
|
Ok a few notable things here:
|
|
Hm now CI is mad about: |
Ah looks like this moved to https://www.linuxfoundation.org/legal/trademark-usage |
|
@nimbinatus this is relevant to your interests too, at least as far as reorganizing the site a bit to add a Blog for community articles |
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request adds a new blog section to the website, including a detailed technical post about container image layers and tar archives. The changes also include adding templates for the blog, and updating the site configuration and main page.
My review focuses on the new template files and the blog post content. I've found a critical issue with a macro used in the new templates that will cause broken links in the header. I've also identified a high-severity bug in the blog listing template related to pagination, which would prevent posts from showing up if pagination is enabled. Additionally, I've provided some suggestions to improve the grammar and readability of the blog post, and to clean up the templates.
|
|
||
| {% block header %} | ||
| <header class="box-shadow"> | ||
| {{ macros::render_header() }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The render_header macro called here appears to be coming from a buggy version in themes/juice/templates/_macros.html. It will generate broken links for the extra menu items (like 'About', 'Docs', 'Blog', etc.). This is a critical issue that breaks site navigation. The macro needs to be fixed to correctly generate the href for links. A correct implementation seems to exist in templates/_macros.html.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hum...I have zero expertise in this stuff but my guess is that Gemini is also guessing on this stuff heuristically.
Does this zola+juice stuff actually compile and sanity check this stuff? If so we can ignore this. If not let's see about fixing that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I don't really understand how the theme stuff works.
There is some amount of checking that zola does to sanity check for broken links or other obviously broken things because I got big scary red errors on the console telling me to fix things. But I don't know how exhaustively it checks.
|
|
||
| {% block header %} | ||
| <header class="box-shadow"> | ||
| {{ macros::render_header() }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The render_header macro called here appears to be coming from a buggy version in themes/juice/templates/_macros.html. It will generate broken links for the extra menu items (like 'About', 'Docs', 'Blog', etc.). This is a critical issue that breaks site navigation. The macro needs to be fixed to correctly generate the href for links. A correct implementation seems to exist in templates/_macros.html.
| </h1> | ||
| <ul> | ||
| <!-- If you are using pagination, section.pages will be empty. You need to use the paginator object --> | ||
| {% for page in section.pages %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the comment on line 14 correctly notes, this loop will not work if pagination is enabled for the blog section, because section.pages will be empty. To fix this and make the template robust, you should use paginator.pages with a fallback to section.pages.
{% for page in paginator.pages | default(value=section.pages) %}
content/blog/2025-dec-15-blog-containers-pitfalls-of-incomplete-tar-archives.md
Outdated
Show resolved
Hide resolved
content/blog/2025-dec-15-blog-containers-pitfalls-of-incomplete-tar-archives.md
Outdated
Show resolved
Hide resolved
| <ul> | ||
| <!-- If you are using pagination, section.pages will be empty. You need to use the paginator object --> | ||
| {% for page in section.pages %} | ||
| <li><a href="{{ page.permalink | safe }}">{{ page.date }} - {{ page.title }}</a></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The safe filter on page.permalink is not necessary because Zola permalinks are already marked as safe. Removing it would make the code cleaner and more consistent with other parts of the templates (e.g., the archive list in the sidebar).
<li><a href="{{ page.permalink }}">{{ page.date }} - {{ page.title }}</a></li>
Signed-off-by: John Eckersberg <jeckersb@redhat.com>
Signed-off-by: John Eckersberg <jeckersb@redhat.com>
|
I don't think the remaining gemini nits are a huge deal. I just copied Also this seems to be incorrect: The links all look fine to me 🤷 |
|
OK let's merge and see! Worst case we revert |
|
It's live and I sanity checked and LGTM |
Signed-off-by: John Eckersberg jeckersb@redhat.com