[TECH-DEBT] Use Empty BuildSpec in `BuildSpecFromFlags` method

[adambkaplan]

From code review:

Seems a bit a weird to me to initialize everything with pointers to empty strings just to be later updated by flag values, if I understand it correctly. Is there any way we can simplify this? What do you think?

Originally posted by @HeavyWombat in #304 (comment)

[rxinui]

/assign

[rxinui]

I'm exploring the codebase to gain confidence and move on slowly to higher priority. Ideally, would like to help on the build or operator asap.

[rxinui]

Hi @HeavyWombat, regarding your comment I can suggest the following:

1. Methods calling struct &buildv1beta1.BuildSpec will omit any field that is initiliased to empty string

For instance, current code:

// BuildSpecFromFlags creates a BuildSpec instance based on command-line flags.
func BuildSpecFromFlags(flags *pflag.FlagSet) (*buildv1beta1.BuildSpec, *string, *string) {
	clusterBuildStrategyKind := buildv1beta1.ClusterBuildStrategyKind
	pruneOption := buildv1beta1.PruneNever
	spec := &buildv1beta1.BuildSpec{
		Source: &buildv1beta1.Source{
			ContextDir: ptr.To(""),
			Git: &buildv1beta1.Git{
				Revision:    ptr.To(""),
				CloneSecret: ptr.To(""),
			},
// ...

will become ->

// BuildSpecFromFlags creates a BuildSpec instance based on command-line flags.
func BuildSpecFromFlags(flags *pflag.FlagSet) (*buildv1beta1.BuildSpec, *string, *string) {
	clusterBuildStrategyKind := buildv1beta1.ClusterBuildStrategyKind
	pruneOption := buildv1beta1.PruneNever
	spec := &buildv1beta1.BuildSpec{
		Source: &buildv1beta1.Source{
			Git: &buildv1beta1.Git{},

The logic of init the flag remain unchanged

  sourceFlags(flags, spec.Source)

This will solve the issue.

2. Replace ptrTo by new(string) explicitly (my recommendation)

// BuildSpecFromFlags creates a BuildSpec instance based on command-line flags.
func BuildSpecFromFlags(flags *pflag.FlagSet) (*buildv1beta1.BuildSpec, *string, *string) {
	clusterBuildStrategyKind := buildv1beta1.ClusterBuildStrategyKind
	pruneOption := buildv1beta1.PruneNever
	spec := &buildv1beta1.BuildSpec{
		Source: &buildv1beta1.Source{
			ContextDir: new(string),
			Git: &buildv1beta1.Git{
				Revision:    new(string),
				CloneSecret: new(string),
			},
// ...

This way we keep visibility of the spec manipulation.

In my humble opinion, ptrTo("") is not bothersome and the method itself comes from k8s/utils lib. It has the merit to explicitly declare the spec, if anything i'd go with proposition 2.

Any other idea?