-
Notifications
You must be signed in to change notification settings - Fork 2.9k
quadlet install: add support for multiple quadlets in a single file #27384
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
base: main
Are you sure you want to change the base?
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: flouthoc The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
6a18d3d to
8ea91d2
Compare
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
6688c20 to
1ea3992
Compare
|
The title of the PR and the release notes comment are misleading. This PR does not add this capability to Quadlet, it adds it to |
1ea3992 to
f90c8a5
Compare
I agree this is only applicable for |
|
Please add the issue link with
Just to be clear I am still against doing this overall but I guess I was outvoted on that so that doesn't matter. |
3ceac95 to
3efc462
Compare
|
@Luap99 I completely agree with you. But, I kinda disengaged from the |
|
@ygalblum there was an internal design doc https://docs.google.com/document/d/1fVjmxVA6k_83iVztQ4j4ISGW7JMVb7yCBfO6vYk9SWs/edit?usp=sharing If that wasn't shared to you then this is already a big issue as you as main quadlet maintainer should be able to look at these things and give your input. As I noted there design docs shouldn't be internal at all and be public in this repo so all maintainers and community people can actually give feedback https://github.com/containers/podman/tree/main/contrib/design-docs which was also not done here.
Well the way I see it decided "somewhere" doesn't count in general, we decide upstream on github what gets merged which includes all maintainers including you of course. So if you think this is a bad idea you can still speak up. I already objected that feature internally and on the issue (#26447) but it seemed like some other maintainers on the team were in favour and others abstained so I didn't push any further against this. |
I clicked the link and landed on a web page saying: There is also a text input field Is the document only for internal use? There is button to click for |
Honny1
left a comment
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.
My only thought is that we could improve performance by reading the input file only once instead of multiple times.
There is no reason why this should be private IMO. I don't own the doc so I cannot share it. As I said for feedback and public knowledge this should have been posted here on github into the design-doc directory instead of being a private doc. There have been a ton of design docs internally over the years. I am trying to push all to do these discussions in public here on github for transparency and to allow for feedback outside our team early on in the design phase. |
|
I don't have access to this doc either. I remember the discussion we had on Quadlet's support for single file and I agree that Quadlet should not support it. Generally speaking about To me, the |
That is a good idea. |
@ygalblum You should have access to design doc now.
I agree, I think we can limit this to one particular extension maybe like |
3efc462 to
5e8c0b9
Compare
3e5da4c to
5c6f512
Compare
e394f15 to
03e35a0
Compare
pkg/domain/infra/abi/quadlet.go
Outdated
| isQuadletsFile := ext == ".quadlets" | ||
|
|
||
| // Only check for multi-quadlet content if it's a .quadlets file | ||
| var isMulti bool | ||
| if isQuadletsFile { | ||
| // For .quadlets files, always treat as multi-quadlet (even single quadlets) | ||
| isMulti = true | ||
| } |
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.
I think isMulti always has the same value as isQuadletsFile.
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.
Done.
Enable installing multiple quadlets from one file using '---' delimiters. Each section requires '# FileName=<name>' comment for custom naming. Single quadlet files remain unchanged for backward compatibility. Assited by: claude-4-sonnet Signed-off-by: flouthoc <flouthoc.git@gmail.com>
03e35a0 to
a1501c4
Compare
|
@Honny1 PTAL |
Honny1
left a comment
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.
LGTM
|
Code LGTM, and I really like making this into a new file type so the old quadlet files are handled as before. I do have a couple of smal tweaks for the doc that I'd like to see happen, and then I'd like a head nod from @ygalblum |
a1501c4 to
c5a0182
Compare
ygalblum
left a comment
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.
Sorry for the delay in my review
| Image=nginx:latest | ||
| ContainerName=web-server | ||
| PublishPort=8080:80 | ||
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.
Are the extra empty lines (before and after the delimiter) required?
pkg/domain/infra/abi/quadlet.go
Outdated
| if assetFile == "" { | ||
| assetFile = "." + filepath.Base(toInstall) + ".asset" | ||
| // Check if this is a .quadlets file - if so, treat as an app | ||
| ext := strings.ToLower(filepath.Ext(toInstall)) |
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.
Why ToLower? We don't do that on any other extensions (see
podman/pkg/systemd/quadlet/unitdirs.go
Line 17 in d3c5c5d
| func IsExtSupported(filename string) bool { |
pkg/domain/infra/abi/quadlet.go
Outdated
|
|
||
| // Check if this file has a supported extension or is a .quadlets file | ||
| hasValidExt := systemdquadlet.IsExtSupported(toInstall) | ||
| ext := strings.ToLower(filepath.Ext(toInstall)) |
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.
same comment regarding ToLower
pkg/domain/infra/abi/quadlet.go
Outdated
| if assetFile != "" { | ||
| // This is part of an app installation, allow non-quadlet files as assets | ||
| // Don't validate as quadlet file (validateQuadletFile will be false) | ||
| } else { |
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.
Don't add empty contexts
| if assetFile != "" { | |
| // This is part of an app installation, allow non-quadlet files as assets | |
| // Don't validate as quadlet file (validateQuadletFile will be false) | |
| } else { | |
| // Allow non-quadlet files as assets | |
| // Don't validate as quadlet file (validateQuadletFile will be false) | |
| if assetFile == "" { |
pkg/domain/infra/abi/quadlet.go
Outdated
| line = strings.TrimSpace(line) | ||
| if strings.HasPrefix(line, "[") && strings.HasSuffix(line, "]") { | ||
| sectionName := strings.ToLower(strings.Trim(line, "[]")) | ||
| switch sectionName { |
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.
You are already missing artifact. This should take a different approach. Something like:
expected := "." + sectionName
if systemdquadlet.IsExtSupported("a" + expectedExt) {
return expected, nil
}
This way when a new extension is added it will work out of the box
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.
Done.
| @@ -0,0 +1,371 @@ | |||
| #!/usr/bin/env bats -*- bats -*- | |||
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.
None of the test use additional systemd sections. Need to make sure that these section both don't interfere with the creation of the standalone files and that they are copied correctly
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.
Done, added in first test.
| local install_dir=$(get_quadlet_install_dir) | ||
|
|
||
| # Create a multi-quadlet file | ||
| local multi_quadlet_file=$PODMAN_TMPDIR/webapp.quadlets |
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.
Using $(random_string) allows parallelism
| local multi_quadlet_file=$PODMAN_TMPDIR/webapp.quadlets | |
| local multi_quadlet_file=$PODMAN_TMPDIR/webapp_$(random_string).quadlets |
Also for the file names set inside the file. To keep track, you'll need to calculate the names outside of the file writing
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.
Done.
| assert "$output" =~ "Image=$IMAGE" "container file should contain correct image" | ||
| assert "$output" =~ "ContainerName=web-server" "container file should contain container name" | ||
|
|
||
| run cat "$install_dir/appstorage.volume" |
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.
Why do you need both cut and podman quadlet print?
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.
Removed.
| assert "$output" =~ "missing required.*FileName" "error should mention missing FileName" | ||
| } | ||
|
|
||
| @test "quadlet verb - multi-quadlet file creates application" { |
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.
Can't you cover this with the first test?
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.
Done.
12cc6f9 to
2415f5b
Compare
2415f5b to
f7f42bb
Compare
Quadlets installed from `.quadlet` file now belongs to a single application, anyone file removed from this application removes all the other files as well. Assited by: claude-4-sonnet Signed-off-by: flouthoc <flouthoc.git@gmail.com>
f7f42bb to
77263fd
Compare
| * Specify a directory containing multiple Quadlet files and other non-Quadlet files for installation ( example a config file for a quadlet container ). | ||
|
|
||
| Note: If a quadlet is part of an application, removing that specific quadlet will remove the entire application. When a quadlet is installed from a directory, all files installed from that directory—including both quadlet and non-quadlet files—are considered part of a single application. | ||
| * Install multiple Quadlets from a single file with the `.quadlets` extension, where each Quadlet is separated by a `---` delimiter. When using multiple quadlets in a single `.quadlets` file, each quadlet section must include a `# FileName=<name>` comment to specify the name for that quadlet, extension of `FileName` is not required as it will be generated by parser internally. |
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.
extension of
FileNameis not required
That's not the case. It's not that the extension is not required. The code fails if it finds an extension.
Having said that, see my comments on that check
| _, err = tmpFile.WriteString(quadlet.content) | ||
| tmpFile.Close() | ||
| if err != nil { | ||
| os.Remove(tmpFile.Name()) |
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.
Remove this line, you've already deferred the call to remove
| installReport.QuadletErrors[toInstall] = fmt.Errorf("unable to write quadlet section %s to temporary file: %w", quadlet.name, err) | ||
| continue | ||
| } | ||
| tmpFile.Close() |
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.
Remove this line, you've already closed the file
| destName := quadlet.name + quadlet.extension | ||
| installedPath, err := ic.installQuadlet(ctx, tmpFile.Name(), destName, installDir, assetFile, true, options.Replace) | ||
| if err != nil { | ||
| os.Remove(tmpFile.Name()) |
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.
Remove this line, you've already deferred the call to remove
| } | ||
|
|
||
| // Clean up temporary file | ||
| os.Remove(tmpFile.Name()) |
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.
Remove this line, you've already deferred the call to remove
| } | ||
|
|
||
| // Extract name for this quadlet section | ||
| var name string |
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.
| var name string | |
| name := baseName |
And remove the else context
|
|
||
| // Extract name for this quadlet section | ||
| var name string | ||
| if isMultiSection { |
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.
So if there's only one section, FileName is not required?
| if strings.ContainsAny(fileName, "/\\") { | ||
| return "", fmt.Errorf("FileName '%s' cannot contain path separators", fileName) | ||
| } | ||
| if strings.Contains(fileName, ".") { |
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.
. is a valid character for a file name regardless to the file's extension. For example the filename ygal.blum.log is a valid filename with .log being the extension. So, not sure if we should validate it.
Add ability to
quadlet installto install multiple quadlets from a single file by separatingthem with
---delimiters, similar to how multiple quadlets can beinstalled from a directory. Each section requires
# FileName=<name>comment for custom naming.Example usage:
Does this PR introduce a user-facing change?