Initializing shallow cloning logic and CRD change #1824
Conversation
pull-request-size
bot
added
the
size/M
There was a problem hiding this comment.
Great ideas here - 👍 to adding this to the API. I have a suggestion on the field naming, plus I noticed the current draft includes a breaking behavior change.
The only items missing IMO are tests. I think this would need the following:
- Unit test(s) to verify we are passing the right arguments to the git clone command.
- End to end test(s) that inspect git history from one of our sample repositories during the build (ex: as a
RUNcommand in a Dockerfile)
| // ShallowCloneDepth specifies the depth of the shallow clone. | ||
| // If not specified or set to 0, a full clone will be performed. | ||
| // Values greater than 0 will create a shallow clone with the specified depth. | ||
| // | ||
| // +optional | ||
| ShallowCloneDepth *int `json:"shallowCloneDepth,omitempty"` |
There was a problem hiding this comment.
I would simplify the name of this field to "Depth". This matches the --depth flag in the git command line.
| // ShallowCloneDepth specifies the depth of the shallow clone. | |
| // If not specified or set to 0, a full clone will be performed. | |
| // Values greater than 0 will create a shallow clone with the specified depth. | |
| // | |
| // +optional | |
| ShallowCloneDepth *int `json:"shallowCloneDepth,omitempty"` | |
| // Depth specifies the git clone depth. If set to 0, cloning will include the full source history. | |
| // Values greater than 0 will create a shallow clone with the specified history depth. | |
| // | |
| // +optional | |
| Depth *int `json:"depth,omitempty"` |
| // If not specified or set to 0, a full clone will be performed. | ||
| // Values greater than 0 will create a shallow clone with the specified depth. | ||
| // | ||
| // +optional |
There was a problem hiding this comment.
I would default this value to 1 so that we do not introduce a breaking change, and add a validation that this must be greater than or equal to 0.
| // +optional | |
| // +optional | |
| // +default=1 | |
| // +kubebuilder:validation:Minimum=0 |
| // Check if shallow clone is requested | ||
| if source.ShallowCloneDepth != nil && *source.ShallowCloneDepth > 0 { | ||
| gitStep.Args = append( | ||
| gitStep.Args, | ||
| "--depth", | ||
| fmt.Sprintf("%d", *source.ShallowCloneDepth), | ||
| ) | ||
| } |
There was a problem hiding this comment.
🤔 if we set a default value of 1, and validate that Depth must be >= 0, can we simply pass the depth value as-is?
There was a problem hiding this comment.
I think we still need the nil-check. I am not 100 % sure whether k8s api server with default=1 will actually set it on create/update operations. But even if so, for existing objects it will be nil.
| pflag.UintVar(&flagValues.depth, "depth", 1, "Create a shallow clone based on the given depth") | ||
| // Setting depth to 0 means no shallow clone (full clone) | ||
| pflag.UintVar(&flagValues.depth, "depth", 0, "Create a shallow clone with the given depth. 0 means full clone (no shallow).") |
There was a problem hiding this comment.
This is a breaking change - current Shipwright users will see a performance hit if we switch from "no history" when cloning to "full history." I recommend reverting this change.
| flagValues = settings{depth: 1} | ||
| flagValues = settings{depth: 0} |
There was a problem hiding this comment.
Likewise - please revert so we don't change the current behavior of cloning source.
| @@ -266,6 +267,7 @@ func clone(ctx context.Context) error { | |||
| cloneArgs = append(cloneArgs, "--branch", flagValues.revision) | |||
| } | |||
|
|
|||
| // Only add depth if it's greater than 0 (meaning shallow clone is requested) | |||
SaschaSchwarze0
added
the
kind/feature
Changes
Submitter Checklist
See the contributor guide
for details on coding conventions, github and prow interactions, and the code review process.
Release Notes