git: add a cache based on bundles #380
Conversation
|
Hello @Zebradil, |
|
Hey @cdevienne, sorry for the delay. As I'm currently not allowed to approve workflow runs on my own, I was waiting for the checks to be completed. But apparently, checks are not emitting any notifications upon completion, so this PR fell out of my sight. I'll check it out today or tomorrow. |
There was a problem hiding this comment.
First of all, many thanks for bringing this topic into attention and filing this PR!
The idea of using bundle seems okay. However, I have a couple of questions about it:
- Did you consider maintaining a working copy in the cache instead of a bundle? (Earlier we had a discussion about the cache size. I see that the bundle approach allows to keep the cache size under control, which is hard when using the clone approach. But are there any other considerations?)
- How does it work when history of a remote git repo gets rewritten (with
git push --force, for example)?
Apart from that I left a few code-related comments. These suggestions should make the code a little more straightforward and easier to change in the future.
@joaopapereira Another thing I noticed is that now the vendir sync output can be quite mouthful, as it prints all refs in the repo being synced two times (in addition to the output of git fetch origin we can see in the main version of vendir).
UPD: This is not an issue of this PR, it is how git command runner works in vendir: it prints full output of a command. I'm not sure how to go about that. Maybe we should make verbosity level configurable in the future.
Signed-off-by: Christophe de Vienne <[email protected]>
Signed-off-by: Christophe de Vienne <[email protected]>
This allow to keep the Sync() signature as it was previously Signed-off-by: Christophe de Vienne <[email protected]>
Yes I did, but it requires a bigger refactoring of the fetch code, and a deeper knowledge of git.
Then the git fetch would get all the new commits and cache a new updated bundle.
Yes that would be nice. |
There was a problem hiding this comment.
LGTM,
except the fact that the public interface is changed (new argument to Retrieve). However, to me it's a minor issue, as I don't think that it'll bring any problems to users.
@joaopapereira could you take a look as well?
do you want to create an issue with that? |
|
I will try to review it next week |
|
The PR looks good, is there any way we can have some tests around the caching? |
Signed-off-by: Christophe de Vienne <[email protected]>
|
I added a basic test to prove it unbundles from the cache. |
Good idea. #387 |
test/e2e/git_test.go
Outdated
| if out, err := exec.Command("tar", "xzvf", "assets/git-repo-signed/asset.tgz", "-C", gitSrcPath).CombinedOutput(); err != nil { | ||
| t.Fatalf("Unpacking git-repo-signed asset: %s (output: '%s')", err, out) | ||
| } |
There was a problem hiding this comment.
| if out, err := exec.Command("tar", "xzvf", "assets/git-repo-signed/asset.tgz", "-C", gitSrcPath).CombinedOutput(); err != nil { | |
| t.Fatalf("Unpacking git-repo-signed asset: %s (output: '%s')", err, out) | |
| } | |
| out, err := exec.Command("tar", "xzvf", "assets/git-repo-signed/asset.tgz", "-C", gitSrcPath).CombinedOutput() | |
| require.NoError(t, err) |
There was a problem hiding this comment.
done, but kept the custom message
Signed-off-by: Christophe de Vienne <[email protected]>
The cache strategy is simpler than the one I did for mercurial, and a little less efficient (it is slower).
But it is solid, and avoids a cumbersome refactoring of the fetcher.