Quadlet - make sure the order of the UnitsDir is deterministic #24062
Conversation
openshift-ci
bot
added
release-note-none
approved
|
Ephemeral COPR build failed. @containers/packit-build please check. |
ygalblum
force-pushed
the
quadlet-restore-dir-order
branch
from
25e8717 to
e1f3a61
Compare
test/system/252-quadlet.bats
Outdated
| dir1=$PODMAN_TMPDIR/02_$(random_string) | ||
| dir2=$PODMAN_TMPDIR/01_$(random_string) |
There was a problem hiding this comment.
I was thinking about this yesterday... and think the original form, without 02/01, is preferable. Your way would catch deterministic sorting; the original way catches a broader class of failures (albeit at the cost of them being flaky and hard to debug).
Would you mind fixing Image as I suggested in the bug report, at least in the dir2 file? Something like Image=YOU-SHOULD-NEVER-SEE-THIS-BAD-BAD-BAD? Just to help future maintainers?
There was a problem hiding this comment.
Thanks for the review. Done and Done.
ygalblum
force-pushed
the
quadlet-restore-dir-order
branch
from
e1f3a61 to
a2c83a8
Compare
test/system/252-quadlet.bats
Outdated
| # If two directories in the search have files with the same name, quadlet should | ||
| # only process the first name | ||
| # prepend the directory name to make test that the order set in QUADLET_UNIT_DIRS is kept |
There was a problem hiding this comment.
Thank you! Sorry to be a stickler but this comment no longer makes any sense
ygalblum
force-pushed
the
quadlet-restore-dir-order
branch
from
a2c83a8 to
2a73637
Compare
cmd/quadlet/main.go
Outdated
| return dirs | ||
| } | ||
|
|
||
| func addDirToDirs(path string, dirs []string, visitedDirs map[string]struct{}) []string { |
There was a problem hiding this comment.
maybe it would have been cleaner to define a helper type
type searchPaths struct {
sorted []string
// map to store paths so we can quickly check if we saw them already and not loop in case of symlinks
visitedDirs map[string]struct{}
}And then define a Add() method on it and pass the struct as pointer around.
But given you have written the code already am I also good with the current version, let me know if you like to change it. If not I will merge it
There was a problem hiding this comment.
Great idea. I'll implement it
Change getUnitDirs to maintain a slice in addition to the map and return the slice Add helper functions to make the code more readable Adjust unit tests Restore system test Signed-off-by: Ygal Blum <[email protected]>
ygalblum
force-pushed
the
quadlet-restore-dir-order
branch
from
2a73637 to
ebbec00
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe, Luap99, ygalblum The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit 87dcf9d
into
containers:main
Does this PR introduce a user-facing change?
No
Resolves: #24047