Change StrategyRef strategy kind default behaviour

[HeavyWombat]

While working on a PR, it was that @qu1queee and I were wondering why the BuildStrategyKind is a pointer, because this eventually leads to nil checks and the implicit internal contract that nil means namespace based build strategy.

Idea: Make the Kind mandatory as in making it not a pointer in the spec. We would force end-users to explicitly decide whether it is a ClusterBuildStrategy or a (namespace) BuildStrategy, which I think makes sense to force that decision at this point. Side effect is fewer lines of code when it comes to verify the field.

[sbose78]

Maybe it was done to have a reasonable default ( ask less, do more ). But, given that folks might be more likely to use ClusterBuildStrategy ( general feeling ), we may go for a mandatory field in there?

…

On Thu, Mar 11, 2021, 09:58 Matthias Diester ***@***.***> wrote: While working on a PR, it was that @qu1queee <https://github.com/qu1queee> and I were wondering why the BuildStrategyKind is a pointer <https://github.com/shipwright-io/build/blob/f5ddd1e70eefe779f6be7b7fe826107fddf4baf1/pkg/apis/build/v1alpha1/buildstrategy.go#L44>, because this eventually leads to nil checks and the implicit internal contract that nil means namespace based build strategy. *Idea:* Make the Kind mandatory as in making it not a pointer in the spec. We would force end-users to explicitly decide whether it is a ClusterBuildStrategy or a (namespace) BuildStrategy, which I think makes sense to force that decision at this point. Side effect is fewer lines of code when it comes to verify the field. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#657>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEFEAGJZTOGNZXL6V5I3CTTDDK7JANCNFSM4ZAN2YQA> .

[imjasonh]

It seems reasonable to me to have a default, rather than making users specify every time. In Tekton, we assume a Task in the same namespace as the TaskRun, over a ClusterTask, and I think that's the less surprising behavior.

In Tekton we've recently sort of shied away from adding more cluster-scoped types, since they can make it hard to upgrade clusters -- all users get the new behavior all at once, which can cause problems. Over time we might get rid of ClusterTask entirely, or at least not immediately bring it into v1. Tekton bundles are more powerful and beyond-cluster-scoped, since they can live in a registry.

I'd personally vote for the default to be the namespaced BuildStrategy, then fallback to the cluster-scoped one. This also makes it easier to remove the cluster-scoped version in the future, if we decide we want to.

[zhangtbj]

Yes, I also agree to have a default value. Right now, we don't have webhook, so it has to be done in the controller logic.

Also FYI, this is the initial discussion about this pointer logic:
#62 (comment)

(Just one year ago, Mar 12th 2020, time flies fast :) )

[imjasonh]

Worth pointing out that we can have a default value and a non-nillable Kind, by using a defaulting webhook to synchronously update "" -> "whatever-default-value" when the request comes in, before being persisted in etcd.

Tekton does this for timeouts today IIRC, with timeout: "" being updated to timeout: "24h" (say), and the specific default value being configured in a ConfigMap so the user can set their own.

[zhangtbj]

Yes, if we want to introduce the webhook, it is easier.

But as discussed before, we didn't plan to use webhook at the beginning, so we set default in the controller logic. :)

[qu1queee]

This was tackle via #641, if the Kindis not specified, we default to a namespaced-scope strategy.