Open jessesuen opened 12 months ago
We need to be careful with this one. I think the read branch is implicitly HEAD if not specified. If read and write branch are the same, a loop can be created. I think. It's been a while since I looked at this. I'll dig into it some tomorrow.
It also should be out of the ordinary to write to the head of the repo's default branch. If branches aren't Stage-specific and promotions at all Stages are writing to the same branch, things get crazy quickly. Suppose "test" writes to main
and then "uat" does the same. Now if the test App points at main
, it's no longer pointing at the commit specified by the test Stage's current Freight -- which makes the test Stage appear unhealthy.
From godocs:
// ReadBranch specifies a particular branch of the repository from which to
// locate contents that will be written to the branch specified by the
// WriteBranch field. This field is optional. In cases where a
// StageStage includes a GitCommit, that commit's ID will supersede the
// value of this field. Therefore, in practice, this field is only used to
// clarify what branch of a repository can be treated as a source of manifests
// or other configuration when a Stage has no subscription to that
// repository.
//
//+kubebuilder:validation:Optional
//+kubebuilder:validation:Pattern=`^(\w+([-/]\w+)*)?$`
ReadBranch string `json:"readBranch"`
// WriteBranch specifies the particular branch of the repository to be
// updated. This is a required field.
//
//+kubebuilder:validation:MinLength=1
//+kubebuilder:validation:Pattern=`^\w+([-/]\w+)*$`
WriteBranch string `json:"writeBranch"`
StageStage
is a typo I will fix.
ReadBranch
docs should note, but do not, that when unspecified, it is equivalent to specifying the default branch -- typically main
or master
. I will fix this.
ReadBranch
and WriteBranch
should never be the same, so they cannot both be optional or the result will too often be that the both reference the same branch, which is an error condition.
If only one can be optional, it should be ReadBranch
because:
In what I suppose is the average case, there will be a subscription to a git repo. The git commit in the Freight, as noted in the godocs, will take precedence over any value in ReadBranch
, meaning putting anything in this field is usually unnecessary.
Keeping WriteBranch
a required field forces the user to be conscious of where their repo write-backs are going.
Tangentially related:
// GitSubscription defines a subscription to a Git repository.
type GitSubscription struct {
// URL is the repository's URL. This is a required field.
//
//+kubebuilder:validation:MinLength=1
//+kubebuilder:validation:Pattern=`^((https?://)|([\w-]+@))([\w\d\.]+)(:[\d]+)?/(.*)$`
RepoURL string `json:"repoURL"`
// Branch references a particular branch of the repository. This is a required
// field.
//
//+kubebuilder:validation:MinLength=1
//+kubebuilder:validation:Pattern=`^\w+([-/]\w+)*$`
Branch string `json:"branch"`
}
Here Branch
can quite easily stand to be optional because it would be completely unsurprising to subscribe to the default branch when no other branch is specified. I will make that so.
We can revisit this when time permits and if it is still relevant after work on #2219 is complete.
fwiw, the built-in prevention of "loop formation" (i.e. stopping you from writing to a branch you also subscribe to) was removed some time ago since the advent of include/exclude filters on git subscriptions has since made it possible, with the right configuration, to write to a branch you also subscribe to without forming a loop.
Tripped on this trying to onboard a new kargo project
writeBranch can be implicitly HEAD of the remote, if user doesn't supply it. This would cut down on our boilerplate and make examples more universally compatible.