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
inrepoconfig: optimize secondary clones #30400
Merged
k8s-ci-robot
merged 2 commits into
kubernetes:master
from
listx:inrepoconfig-optimizations
Aug 18, 2023
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -88,6 +88,7 @@ type cacher interface { | |||||
type cloner interface { | ||||||
// Clone clones the repository from a local path. | ||||||
Clone(from string) error | ||||||
CloneWithRepoOpts(from string, repoOpts RepoOpts) error | ||||||
} | ||||||
|
||||||
// MergeOpt holds options for git merge operations. | ||||||
|
@@ -139,10 +140,46 @@ func (i *interactor) IsDirty() (bool, error) { | |||||
|
||||||
// Clone clones the repository from a local path. | ||||||
func (i *interactor) Clone(from string) error { | ||||||
return i.CloneWithRepoOpts(from, RepoOpts{}) | ||||||
} | ||||||
|
||||||
// CloneWithRepoOpts clones the repository from a local path, but additionally | ||||||
// use any repository options (RepoOpts) to customize the clone behavior. | ||||||
func (i *interactor) CloneWithRepoOpts(from string, repoOpts RepoOpts) error { | ||||||
i.logger.Infof("Creating a clone of the repo at %s from %s", i.dir, from) | ||||||
if out, err := i.executor.Run("clone", from, i.dir); err != nil { | ||||||
cloneArgs := []string{"clone"} | ||||||
|
||||||
if repoOpts.ShareObjectsWithSourceRepo { | ||||||
cloneArgs = append(cloneArgs, "--shared") | ||||||
} | ||||||
|
||||||
// Handle sparse checkouts. | ||||||
if repoOpts.SparseCheckoutDirs != nil { | ||||||
cloneArgs = append(cloneArgs, "--sparse") | ||||||
} | ||||||
|
||||||
cloneArgs = append(cloneArgs, []string{from, i.dir}...) | ||||||
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.
Suggested change
|
||||||
|
||||||
if out, err := i.executor.Run(cloneArgs...); err != nil { | ||||||
return fmt.Errorf("error creating a clone: %w %v", err, string(out)) | ||||||
} | ||||||
|
||||||
// For sparse checkouts, we have to do some additional housekeeping after | ||||||
// the clone is completed. We use Git's global "-C <directory>" flag to | ||||||
// switch to that directory before running the "sparse-checkout" command, | ||||||
// because otherwise the command will fail (because it will try to run the | ||||||
// command in the $PWD, which is not the same as the just-created clone | ||||||
// directory (i.dir)). | ||||||
if repoOpts.SparseCheckoutDirs != nil { | ||||||
if len(repoOpts.SparseCheckoutDirs) == 0 { | ||||||
return nil | ||||||
} | ||||||
sparseCheckoutArgs := []string{"-C", i.dir, "sparse-checkout", "set"} | ||||||
sparseCheckoutArgs = append(sparseCheckoutArgs, repoOpts.SparseCheckoutDirs...) | ||||||
if out, err := i.executor.Run(sparseCheckoutArgs...); err != nil { | ||||||
return fmt.Errorf("error setting it to a sparse checkout: %w %v", err, string(out)) | ||||||
} | ||||||
} | ||||||
return nil | ||||||
} | ||||||
|
||||||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Maybe we should just always return an "unsupported" error from this function then. That way if it is accidentally used, the problem won't be subtle.
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.
I did try this already, but then we have to delete one of our tests because it will fail:
test-infra/prow/config/inrepoconfig_test.go
Line 35 in 1e81b5a
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.
I think we need to remove that test then, right? If that causes the test to fail that is because this is a breaking change and we don't support the prow YAML getter with the v1 client anymore.
So I think we either need to always return an error from the unsupported function and remove the obsolete test, or properly implement this in the v1 client.
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.
I mean, technically the v1 client is supported for the prow YAML getter. But it will be less optimized than the v2 client, because the v1 client won't use the --sparse or --shared flags.
Granted, we are going to try to disable repo serialization altogether, so when we cross that bridge (soon), the v1 client will be basically unusable for the prow YAML getter. So I am OK with removing the obsolete test. Will update (err, this PR is merged now, so will have to do it as a follow-up).
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.
That followup is part of #30428. It's kind of blocked on #30520 though.