Quadlet installation code refactoring#28860
Conversation
github-actions
Bot
added
the
kind/api-change
l0rd
force-pushed
the
refactor/quadlet-applications-with-fix
branch
3 times, most recently
from
3599897 to
5703e04
Compare
l0rd
changed the title
|
@simonbrauner @inknos PTAL |
l0rd
force-pushed
the
refactor/quadlet-applications-with-fix
branch
2 times, most recently
from
1661876 to
24903e0
Compare
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
inknos
left a comment
inknos
left a comment
There was a problem hiding this comment.
2 nits and a clarification, then LGTM
| // First, validate that the source path exists and is a file | ||
| stat, err := os.Stat(path) | ||
| _, err := os.Stat(srcPath) | ||
| if err != nil { | ||
| return "", fmt.Errorf("quadlet to install %q does not exist or cannot be read: %w", path, err) | ||
| } | ||
| if stat.IsDir() { | ||
| dirs, err := os.ReadDir(path) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| for _, d := range dirs { | ||
| nInstallDir := filepath.Join(installDir, destName) | ||
| err := os.MkdirAll(nInstallDir, 0o755) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| _, err = ic.installQuadlet( | ||
| ctx, | ||
| filepath.Join(path, d.Name()), // path | ||
| d.Name(), // destName | ||
| nInstallDir, // installDir | ||
| replace) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| } | ||
| return path, nil | ||
| return "", fmt.Errorf("quadlet to install %q does not exist or cannot be read: %w", srcPath, err) |
There was a problem hiding this comment.
nit. this comment is outdated
Edit: got me confused when I was reading the code, had to go through it twice :)
There was a problem hiding this comment.
Which comment is outdated?
There was a problem hiding this comment.
// First, validate that the source path exists and is a file
code seemed just clear as is
There was a problem hiding this comment.
I'll be off in the next days, so to be clear, this is non blocking and the PR LGTM in the current state. waiting for tests only
There was a problem hiding this comment.
A few lines after
// First, validate that the source path exists and is a file
there is
// Second, create the destPath folder as it may not exist
So I am reluctant to remove only the first comment and keep the second as is.
l0rd
force-pushed
the
refactor/quadlet-applications-with-fix
branch
from
24903e0 to
9890a3b
Compare
simonbrauner
left a comment
simonbrauner
left a comment
There was a problem hiding this comment.
Checked #28335 (comment), it is fixed.
Added 2 comments, otherwise, LGTM
l0rd
force-pushed
the
refactor/quadlet-applications-with-fix
branch
from
9890a3b to
d7f3d95
Compare
| for _, section := range sections { | ||
| installedPath, err := installMultiQuadletSection(ctx, ic, section, sectionsDestDir, options.Replace) | ||
| if err != nil { | ||
| installReport.QuadletErrors[quadletPath.src] = fmt.Errorf("unable to create temporary file for quadlet section %s: %w", section.name, err) |
There was a problem hiding this comment.
This error message is duplicate/misleading. When there is an error it prints either
unable to create temporary file for quadlet section fromquadlets2: unable to create temporary file for quadlet section fromquadlets2
or
unable to create temporary file for quadlet section fromquadlets2: unable to write quadlet section fromquadlets2 to temporary file
diff --git a/pkg/domain/infra/abi/quadlet.go b/pkg/domain/infra/abi/quadlet.go
index 6b1fcd247a..329799c609 100644
--- a/pkg/domain/infra/abi/quadlet.go
+++ b/pkg/domain/infra/abi/quadlet.go
@@ -159,7 +159,7 @@ func (ic *ContainerEngine) QuadletInstall(ctx context.Context, pathsOrURLs []str
for _, section := range sections {
installedPath, err := installMultiQuadletSection(ctx, ic, section, sectionsDestDir, options.Replace)
if err != nil {
- installReport.QuadletErrors[quadletPath.src] = fmt.Errorf("unable to create temporary file for quadlet section %s: %w", section.name, err)
+ installReport.QuadletErrors[quadletPath.src] = fmt.Errorf("unable to install multi-quadlet section: %w", err)
continue
}
// Record the installation (use a unique key for each section)- Cleanup the code to install quadlets - Fix `podman quadlet install` output message (see podman-container-tools#28335 (comment)) - Update libpod quadlet endpoint documentation Signed-off-by: Mario Loriedo <mario.loriedo@gmail.com>
l0rd
force-pushed
the
refactor/quadlet-applications-with-fix
branch
from
d7f3d95 to
d1e2692
Compare
simonbrauner
left a comment
simonbrauner
left a comment
There was a problem hiding this comment.
My comments were addressed, LGTM
3 participants
This is a follow-up of #28335 with some improvements:
--application)podman quadlet installoutput message (see refactor: podman quadlet sub-command #28335 (comment))Checklist
Ensure you have completed the following checklist for your pull request to be reviewed:
commits. (
git commit -s). (If needed, usegit commit -s --amend). The author email must matchthe sign-off email address. See CONTRIBUTING.md
for more information.
Fixes: #00000in commit message (if applicable)make validatepr(format/lint checks)Noneif no user-facing changes)Does this PR introduce a user-facing change?