-
Notifications
You must be signed in to change notification settings - Fork 1.4k
git url: support CSV form (also support verifying a tag with a commit hash) #5903
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ const AttrAuthHeaderSecret = "git.authheadersecret" | |
| const AttrAuthTokenSecret = "git.authtokensecret" | ||
| const AttrKnownSSHHosts = "git.knownsshhosts" | ||
| const AttrMountSSHSock = "git.mountsshsock" | ||
| const AttrCommitHash = "git.commithash" | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (Bikeshedding: this could be also named
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @tonistiigi Which one do you prefer?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AttrGitChecksum
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Implemented in |
||
| const AttrLocalSessionID = "local.session" | ||
| const AttrLocalUniqueID = "local.unique" | ||
| const AttrIncludePatterns = "local.includepattern" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -92,6 +92,8 @@ func (gs *gitSource) Identifier(scheme, ref string, attrs map[string]string, pla | |
| id.KnownSSHHosts = v | ||
| case pb.AttrMountSSHSock: | ||
| id.MountSSHSock = v | ||
| case pb.AttrCommitHash: | ||
| id.CommitHash = v | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -207,6 +209,9 @@ func (gs *gitSourceHandler) shaToCacheKey(sha, ref string) string { | |
| if gs.src.Subdir != "" { | ||
| key += ":" + gs.src.Subdir | ||
| } | ||
| if gs.src.CommitHash != "" { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this needed? Isn't
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No,
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does that matter, if we already have a complete one. Same as https://github.com/moby/buildkit/pull/5903/files/076850aeb7720b56683710d8e30b05fbb9352b97#diff-bf1789b0b19c8d6533a10da5e27f0b31da2cdcd37510a787498c3d19a985d470 . Eg. if you build from
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding a new cache key seems safer to enforce hash verification, as an existing cache might have been populated without verifying the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can continue the discussion in |
||
| key += ":commit-hash=" + gs.src.CommitHash | ||
| } | ||
| return key | ||
| } | ||
|
|
||
|
|
@@ -349,10 +354,18 @@ func (gs *gitSourceHandler) CacheKey(ctx context.Context, g session.Group, index | |
| gs.locker.Lock(remote) | ||
| defer gs.locker.Unlock(remote) | ||
|
|
||
| if ref := gs.src.Ref; ref != "" && gitutil.IsCommitSHA(ref) { | ||
| cacheKey := gs.shaToCacheKey(ref, "") | ||
| var refCommitFullHash string | ||
| if gitutil.IsCommitSHA(gs.src.CommitHash) { | ||
| refCommitFullHash = gs.src.CommitHash | ||
| } | ||
| if refCommitFullHash == "" && gitutil.IsCommitSHA(gs.src.Ref) { | ||
| refCommitFullHash = gs.src.Ref | ||
| } | ||
| if refCommitFullHash != "" { | ||
| cacheKey := gs.shaToCacheKey(refCommitFullHash, "") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The ref should be set here if one is present even if we know the full hash. If first build is from
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| gs.cacheKey = cacheKey | ||
| return cacheKey, ref, nil, true, nil | ||
| // gs.src.CommitHash is verified after checking out the commit | ||
| return cacheKey, refCommitFullHash, nil, true, nil | ||
| } | ||
|
|
||
| gs.getAuthToken(ctx, g) | ||
|
|
@@ -415,7 +428,9 @@ func (gs *gitSourceHandler) CacheKey(ctx context.Context, g session.Group, index | |
| if !gitutil.IsCommitSHA(sha) { | ||
| return "", "", nil, false, errors.Errorf("invalid commit sha %q", sha) | ||
| } | ||
|
|
||
| if gs.src.CommitHash != "" && !strings.HasPrefix(sha, gs.src.CommitHash) { | ||
| return "", "", nil, false, errors.Errorf("expected commit hash to match %s, got %s", gs.src.CommitHash, sha) | ||
| } | ||
| cacheKey := gs.shaToCacheKey(sha, usedRef) | ||
| gs.cacheKey = cacheKey | ||
| return cacheKey, sha, nil, true, nil | ||
|
|
@@ -536,6 +551,7 @@ func (gs *gitSourceHandler) Snapshot(ctx context.Context, g session.Group) (out | |
| subdir = "." | ||
| } | ||
|
|
||
| checkedoutRef := "HEAD" | ||
| if gs.src.KeepGitDir && subdir == "." { | ||
| checkoutDirGit := filepath.Join(checkoutDir, ".git") | ||
| if err := os.MkdirAll(checkoutDir, 0711); err != nil { | ||
|
|
@@ -605,6 +621,7 @@ func (gs *gitSourceHandler) Snapshot(ctx context.Context, g session.Group) (out | |
| if err != nil { | ||
| return nil, errors.Wrapf(err, "failed to checkout remote %s", urlutil.RedactCredentials(gs.src.Remote)) | ||
| } | ||
| checkedoutRef = ref // HEAD may not exist | ||
| if subdir != "." { | ||
| d, err := os.Open(filepath.Join(cd, subdir)) | ||
| if err != nil { | ||
|
|
@@ -635,6 +652,16 @@ func (gs *gitSourceHandler) Snapshot(ctx context.Context, g session.Group) (out | |
| } | ||
|
|
||
| git = git.New(gitutil.WithWorkTree(checkoutDir), gitutil.WithGitDir(gitDir)) | ||
| if gs.src.CommitHash != "" { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this happening after we have already completed the checkout instead of before?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just for verification, as we may potentially have an unexpected code path (in future) |
||
| actualHashBuf, err := git.Run(ctx, "rev-parse", checkedoutRef) | ||
| if err != nil { | ||
| return nil, errors.Wrapf(err, "failed to rev-parse %s for %s", checkedoutRef, urlutil.RedactCredentials(gs.src.Remote)) | ||
| } | ||
| actualHash := strings.TrimSpace(string(actualHashBuf)) | ||
| if !strings.HasPrefix(actualHash, gs.src.CommitHash) { | ||
| return nil, errors.Errorf("expected commit hash to match %s, got %s", gs.src.CommitHash, actualHash) | ||
| } | ||
| } | ||
| _, err = git.Run(ctx, "submodule", "update", "--init", "--recursive", "--depth=1") | ||
| if err != nil { | ||
| return nil, errors.Wrapf(err, "failed to update submodules for %s", urlutil.RedactCredentials(gs.src.Remote)) | ||
|
|
||
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.
(Use
git show --ignore-all-spaceto review)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.
This part was moved to: