- 
                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.