-
Notifications
You must be signed in to change notification settings - Fork 85
Add support for Git sparse checkout when .spec.update.path is specified
#920
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
Add support for Git sparse checkout when .spec.update.path is specified
#920
Conversation
2dd1868 to
0dfd5e8
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.
Thanks for this improvement. I would like to see this this feature aligned with the API for GitRepository resources that we implemented in fluxcd/source-controller#1774 and make it an explicit field in the API instead of a feature flag of the controller. Not sure how @stefanprodan or @matheuscscp think about this.
We don't have a |
0dfd5e8 to
8be4dad
Compare
internal/features/features.go
Outdated
|
|
||
| // GitSparseCheckout | ||
| // opt-out from v0.42 | ||
| GitSparseCheckout: true, |
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.
Let's make this opt-in for now. We can switch this to opt-out once sparse checkout is no longer an experimental feature in Git.
| // When a path is specified, sparse checkout will be enabled to limit the checkout scope | ||
| // to the specified directory, reducing clone size and time. |
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 would remove this, since we'll have sparse checkout as an opt-in flag.
8be4dad to
d73a337
Compare
|
@stefanprodan |
|
Thanks @kane8n for this contribution! I'm putting this on hold as we need to do a patch release today. After that I'm going to ask you to rebase and then we can merge it. This feature will be included in Flux 2.7 |
|
@kane8n please rebase |
d73a337 to
5ceb013
Compare
|
@stefanprodan |
internal/source/source.go
Outdated
| } | ||
| } | ||
|
|
||
| // WithCheckoutOptionRecurseSubmodules is a CheckoutOption option to configure |
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.
| // WithCheckoutOptionRecurseSubmodules is a CheckoutOption option to configure | |
| // WithCheckoutOptionSparseCheckoutDirectories is a CheckoutOption option to configure |
internal/source/source.go
Outdated
| // SparseCheckoutDirectories. | ||
| func WithCheckoutOptionSparseCheckoutDirectories(updatePath string) CheckoutOption { | ||
| return func(cc *repository.CloneConfig) { | ||
| cc.SparseCheckoutDirectories = []string{filepath.Clean(updatePath)} |
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 skip enabling sparse checkout when the path is the repo root e.g. path: ./
Signed-off-by: kane8n <[email protected]>
5ceb013 to
3999c65
Compare
|
@stefanprodan |
| func WithCheckoutOptionSparseCheckoutDirectories(updatePath string) CheckoutOption { | ||
| return func(cc *repository.CloneConfig) { | ||
| cleanedPath := filepath.Clean(updatePath) | ||
| if cleanedPath == "." { |
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.
We need to remove the ./ prefix as sparse checkout will fail due to a bug in go-git. See https://github.com/fluxcd/source-controller/blob/97c995b8c8e3a6c9c31cb4f1e8323428a03d07d3/internal/controller/gitrepository_controller.go#L976
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.
@stefanprodan
Use filepath.Clean to remove the leading ./ prefix.
Should I use the same implementation as source-controller for consistency?
sample
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.
Ok cool, let's leave it like this, SC could drop the custom code and use filepath.Clean
.spec.update.path is specified
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.
LGTM
Thanks @kane8n 🏅
PS. Please open a PR on the website repo to add the feature gate in here: https://github.com/fluxcd/website/blob/main/content/en/flux/components/image/options.md#feature-gates
Summary
If Spec.Update.Path is specified, only the manifest under this Path is required, no other files.
Therefore, SparseCheckout is valid.
Set the Path specified by Spec.Update.Path to SparseCheckoutDirectory so that only the necessary files are downloaded.
Feature Gate
To enable this feature, the controller must be started with the flag
--feature-gates=GitSparseCheckout=true.