-
Notifications
You must be signed in to change notification settings - Fork 379
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
directory: simplify newImageDestination #2520
base: main
Are you sure you want to change the base?
Conversation
LGTM |
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.
Thanks!
The optimization makes sense.
I’m not entirely sure about inlining all of the logic — most of this is really not so core to the newImageDestination
“constructor” to be the primary focus of the function. Maybe the removeDirContents
loop could stay out of line, it’s pretty self-contained. Or maybe all of this should be in a separate initializeDestinationDir
… but let’s get the logic precisely correct first, and then find the right abstraction boundary.
As discussed elsewhere, I’d prefer to keep the error wrapping with "%q", path
, throughout.
f841ecb
to
58e46d0
Compare
The current code is unnecessarily complicated: - it does unnecessary checks if a file/directory exists before opening it; - it uses three helper functions not used anywhere else; - directory contents is read twice. Untangle all this, making the code simpler. Also, move the logic of initializing a destination directory to a separate function. Now, since the logic of handing version file is now encapsulated in a single function, remove versionPath method and const version, as well as the corresponding test. This also fixes the subtle issue of using ref.path for constructing the version file path, but ref.resolvedPath for all other operations. Signed-off-by: Kir Kolyshkin <[email protected]>
This needs to be "require" rather than "assert", as if error is returned, test needs to fail now. Otherwise, close will panic. Signed-off-by: Kir Kolyshkin <[email protected]>
58e46d0
to
e77668f
Compare
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.
ACK on the logic, just some maintenance/organization nits.
return ErrNotContainerImageDir | ||
} | ||
// Indeed an image directory. Reuse by removing all the old contents | ||
// (including the versionFile, which will be recreated below). |
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.
It seems worth it to preserve the rationale “so that only one image is in the directory at a time”.
(OTOH containers/skopeo#1237 exists, but that’s clearly not this PR.)
@@ -190,8 +190,3 @@ func (ref dirReference) signaturePath(index int, instanceDigest *digest.Digest) | |||
} | |||
return filepath.Join(ref.path, fmt.Sprintf("signature-%d", index+1)), nil | |||
} | |||
|
|||
// versionPath returns a path for the version file within a directory using our conventions. | |||
func (ref dirReference) versionPath() 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.
Keeping the "version"
path logic in this place, beside the other …Path
locations, might be a good way to sort-of document the file format? Also, eventually, #1876 will need this to live outside of initDestDir
.
"github.com/opencontainers/go-digest" | ||
"github.com/sirupsen/logrus" | ||
) | ||
|
||
const version = "Directory Transport Version: 1.1\n" |
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 way I think about it, this is really a constant of the file format, not a private implementation detail of initDestDir
(we can’t just change the contents to make initDestDir
individually nicer, for example); so it should be a global constant in the module. Also, eventually, #1876 will need this to live outside of initDestDir
.
[Well, we should eventually actually parse the numbers and do comparisons, but that’s not yet necessary.]
Based on patches from #2512.
The current code is unnecessarily complicated:
Untangle all this, making the code simpler.
Also, move the logic of initializing a destination directory to a
separate function.
Now, since the logic of handing version file is now encapsulated in a
single function, remove versionPath method and const version, as well
as the corresponding test.
This also fixes the subtle issue of using ref.path for constructing the
version file path, but ref.resolvedPath for all other operations.