tart pull: introduce --populate-cache flag #103
Conversation
Sources/tart/Commands/Push.swift
Outdated
| defaultLogger.appendNewLine("caching \(localName) as \(pushedToRemoteName)...") | ||
|
|
||
| let ociVMDir = try VMStorageOCI().create(pushedToRemoteName, overwrite: true) | ||
| try localVMDir.clone(to: ociVMDir, generateMAC: false) |
There was a problem hiding this comment.
I think Tart should only clone once into the shaXXX folder and then create symlinks for all the pushed fully qualified remote names. 🤔
c00fc09 to
eb35fa5
Compare
6cc5402 to
b9432be
Compare
b9432be to
c5de388
Compare
Sources/tart/Commands/Push.swift
Outdated
| let manifestDigest = try await localVMDir.pushToRegistry(registry: registry, references: remoteNamesForRegistry.map{ $0.reference.value }) | ||
|
|
||
| try await localVMDir.pushToRegistry(registry: registry, references: remoteNamesForRegistry.map{ $0.reference }) | ||
| // Populate the local cache (if requested) | ||
| if populateCache { | ||
| for remoteName in remoteNamesForRegistry { | ||
| defaultLogger.appendNewLine("caching \(localName) as \(remoteName)...") | ||
|
|
||
| if let cacheToVMDir = try VMStorageOCI().cache(name: remoteName, digest: manifestDigest) { | ||
| try localVMDir.clone(to: cacheToVMDir, generateMAC: false) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This logic looks a bit too complex. And I still think there is too much cloning is going on. What if pushToRegistry will return a RemoteName with the shaXXX` then the cache population logic might be much simpler:
let digestRemoteName = try await localVMDir.pushToRegistry(registry: registry, references: remoteNamesForRegistry.map{ $0.reference.value })
if populateCache {
let ociStorage = VMStorageOCI()
let digestVMDir = ociStorage.create(digestRemoteName)
try localVMDir.clone(to: digestVMDir, generateMAC: false)
for remoteName in remoteNamesForRegistry {
ociStorage.link(from: remoteName, to: digestVMDir)
}
}There was a problem hiding this comment.
And I still think there is too much cloning is going on.
It only clones if the "digest" directory doesn't exist, otherwise creates a symbolic link. This is achieved by re-using the pull() code.
What if
pushToRegistrywill return aRemoteName with theshaXXX` then the cache population logic might be much simpler:
I see two downsides to this:
- It seems that the
localVMDir.clone()will always clone. - Not reusing the
pull()'s logic results in needless duplication.
There was a problem hiding this comment.
I don't think there will much "needless duplication". 🤷♂️ Here is my take #107
|
|
||
| struct Set: AsyncParsableCommand { | ||
| static var configuration = CommandConfiguration(abstract: "Modify VM's configuration") | ||
| struct SetCommand: AsyncParsableCommand { |
There was a problem hiding this comment.
All the files in Commands folder doesn't have Command suffix. What's the reasoning for this particular rename?
There was a problem hiding this comment.
What's the reasoning for this particular rename?
Set command collides with the Set collection.
There was a problem hiding this comment.
I don't think it's in the scope of this PR. There is no usage of Set. IMO we shouild either rename all the commands or wait util there will be a collision.
Resolves #39.