Skip to content

quadlet API: fix tar install when non-quadlet file sorts first#28696

Open
inknos wants to merge 1 commit into
containers:mainfrom
inknos:fix-quadlet-file-ordering-in-tar
Open

quadlet API: fix tar install when non-quadlet file sorts first#28696
inknos wants to merge 1 commit into
containers:mainfrom
inknos:fix-quadlet-file-ordering-in-tar

Conversation

@inknos
Copy link
Copy Markdown
Collaborator

@inknos inknos commented May 13, 2026

QuadletInstall expects the quadlet file to be first in the file list, The API handler passes files in filepath.Walk order (lexicographic). Install would fail if a file like "Containerfile" comes before the quadlet file.

Checklist

Ensure you have completed the following checklist for your pull request to be reviewed:

  • Certify you wrote the patch or otherwise have the right to pass it on as an open-source patch by signing all
    commits. (git commit -s). (If needed, use git commit -s --amend). The author email must match
    the sign-off email address. See CONTRIBUTING.md
    for more information.
  • Referenced issues using Fixes: #00000 in commit message (if applicable)
  • Tests have been added/updated (or no tests are needed)
  • Documentation has been updated (or no documentation changes are needed)
  • All commits pass make validatepr (format/lint checks)
  • Release note entered in the section below (or None if no user-facing changes)

Does this PR introduce a user-facing change?

None

@github-actions github-actions Bot added the kind/api-change Change to remote API; merits scrutiny label May 13, 2026
@inknos
Copy link
Copy Markdown
Collaborator Author

inknos commented May 13, 2026

@simonbrauner , if you want to take a look at this after what we discussed today, I decided to touch the api function instead of the QuadletInstall function

Copy link
Copy Markdown
Member

@Honny1 Honny1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, once CI is green.

Comment thread test/apiv2/36-quadlets.at
quadlet_5=quadlet-test-5-$(cat /proc/sys/kernel/random/uuid).container
containerfile_1=quadlet-test-containerfile-1-$(cat /proc/sys/kernel/random/uuid).Containerfile
containerfile_1=quadlet-test-containerfile-1-$(cat /proc/sys/kernel/random/uuid)
configfile_1=quadlet-test-configfile-1-$(cat /proc/sys/kernel/random/uuid).conf
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this file removed on cleanup?

Comment thread test/apiv2/36-quadlets.at
tar --format=posix -C "$TMPD" -cvf "$TMPD/$quadlet_7$containerfile_3.tar" "$quadlet_7" "$containerfile_3" &> /dev/null

t POST "libpod/quadlets" "$TMPD/$quadlet_7$containerfile_3.tar" 200 \
'.InstalledQuadlets|length=2' \
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this correct?

Comment thread test/apiv2/36-quadlets.at

t GET "libpod/quadlets/$quadlet_7/file" 200
is "$output" "$quadlet_7_content" "quadlet-7 should be installed"
is "$(cat "$quadlet_install_dir/$containerfile_3")" "$containerfile_3_content" "containerfile_3 should be installed"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should ' $ configfile_1 ' also be checked?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixing your comments, thanks

QuadletInstall expects the quadlet file to be first in the file list,
The API handler passes files in filepath.Walk order (lexicographic).
Install would fail if a file like "Containerfile" comes before the
quadlet file.

Signed-off-by: Nicola Sella <nsella@redhat.com>
@inknos inknos force-pushed the fix-quadlet-file-ordering-in-tar branch from 719b8e2 to 9f7216b Compare May 14, 2026 14:16
Copy link
Copy Markdown
Member

@Honny1 Honny1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@packit-as-a-service
Copy link
Copy Markdown

[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/api-change Change to remote API; merits scrutiny

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants