branch on git subs can be implicitly HEAD when not specified

[jessesuen]

Tripped on this trying to onboard a new kargo project

.writeBranch: Invalid value: "": spec.promotionMechanisms.gitRepoUpdates[0].writeBranch in body should be at least 1 chars long

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.

[krancour]

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.

[krancour]

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.

[krancour]

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.

[krancour]

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.

[github-actions]

This issue has been automatically marked as stale because it had no activity for 90 days. It will be closed if no activity occurs in the next 30 days but can be reopened if it becomes relevant again.

[krancour]

This issue hasn't been relevant for some time. Around the same time Warehouses gained all sorts of path filters, these guardrails were removed. And since that time, we've also moved on from the legacy promotion mechanisms to discrete promotion steps that are easier to understand and reason over. The current behavior matches what any user of a git client would expect. When you commit, you commit to whatever branch is currently checked out. When you push, you push to that same branch unless otherwise specified. Closing as no longer relevant.