Skip to content

Commit

Permalink
Ensure we default to a namespaced strategy
Browse files Browse the repository at this point in the history
When a Build do not specify an strategy Kind, ensure we default to a
namespaced scope one. This logic was already in placed, a previous
commit removed and this commit added it again in a different function.

Also, it should fix shipwright-io#657
  • Loading branch information
qu1queee committed Mar 22, 2021
1 parent 931cecb commit 16f1dd9
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 11 deletions.
1 change: 0 additions & 1 deletion docs/buildrun.md
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,6 @@ The following table illustrates the different states a BuildRun can have under i
| False | Failed | Yes | The BuildRun failed in one of the steps. |
| False | BuildRunTimeout | Yes | The BuildRun timed out. |
| False | UnknownStrategyKind | Yes | The Build specified strategy Kind is unknown. (_options: ClusterBuildStrategy or BuildStrategy_) |
| False | StrategyKindIsMissing | Yes | The Build strategy Kind was not specified. (_options: ClusterBuildStrategy or BuildStrategy_) |
| False | ClusterBuildStrategyNotFound | Yes | The referenced cluster strategy was not found in the cluster. |
| False | BuildStrategyNotFound | Yes | The referenced namespaced strategy was not found in the cluster. |
| False | SetOwnerReferenceFailed | Yes | Setting ownerreferences from the BuildRun to the related TaskRun failed. |
Expand Down
17 changes: 11 additions & 6 deletions pkg/reconciler/buildrun/buildrun.go
Original file line number Diff line number Diff line change
Expand Up @@ -372,14 +372,19 @@ func (r *ReconcileBuildRun) VerifyRequestName(ctx context.Context, request recon
}

func (r *ReconcileBuildRun) getReferencedStrategy(ctx context.Context, build *buildv1alpha1.Build, buildRun *buildv1alpha1.BuildRun) (strategy buildv1alpha1.BuilderStrategy, err error) {
if build.Spec.StrategyRef.Kind == nil {
err = fmt.Errorf("illegal state, strategy kind is unset")

if updateErr := resources.UpdateConditionWithFalseStatus(ctx, r.client, buildRun, fmt.Errorf("undefined strategy Kind").Error(), resources.ConditionStrategyKindIsMissing); updateErr != nil {
return nil, resources.HandleError("failed to get referenced strategy", err, updateErr)
if build.Spec.StrategyRef.Kind == nil {
// If the strategy Kind is not specified, we default to a namespaced-scope strategy
ctxlog.Info(ctx, "missing strategy Kind, defaulting to a namespaced-scope one", buildRun.Name, build.Name, namespace)
strategy, err = resources.RetrieveBuildStrategy(ctx, r.client, build)
if err != nil {
if apierrors.IsNotFound(err) {
if updateErr := resources.UpdateConditionWithFalseStatus(ctx, r.client, buildRun, err.Error(), resources.BuildStrategyNotFound); updateErr != nil {
return nil, resources.HandleError("failed to get referenced strategy", err, updateErr)
}
}
}

return nil, err
return strategy, err
}

switch *build.Spec.StrategyRef.Kind {
Expand Down
9 changes: 5 additions & 4 deletions pkg/reconciler/buildrun/buildrun_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -759,7 +759,7 @@ var _ = Describe("Reconcile BuildRun", func() {
Expect(client.StatusCallCount()).To(Equal(2))
})

It("fails on a TaskRun creation due to undefined buildStrategy kind", func() {
It("fails on a TaskRun creation due to an undefined default strategy kind", func() {
// use a Build object that does not defines the strategy Kind field
buildSample = ctl.BuildWithoutStrategyKind(buildName, strategyName)

Expand All @@ -773,12 +773,13 @@ var _ = Describe("Reconcile BuildRun", func() {

// Stub that asserts the BuildRun status fields when
// Status updates for a BuildRun take place
// We fail here because the default strategy was not found
statusCall := ctl.StubBuildRunStatus(
"undefined strategy Kind",
fmt.Sprintf(" \"%v\" not found", strategyName),
emptyTaskRunName,
build.Condition{
Type: build.Succeeded,
Reason: "StrategyKindIsMissing",
Reason: "BuildStrategyNotFound",
Status: corev1.ConditionFalse,
},
corev1.ConditionFalse,
Expand All @@ -790,7 +791,7 @@ var _ = Describe("Reconcile BuildRun", func() {
// we mark the BuildRun as Failed and do not reconcile again
_, err := reconciler.Reconcile(buildRunRequest)
Expect(err).ToNot(HaveOccurred())
Expect(client.GetCallCount()).To(Equal(4))
Expect(client.GetCallCount()).To(Equal(5))
Expect(client.StatusCallCount()).To(Equal(2))
})

Expand Down

0 comments on commit 16f1dd9

Please sign in to comment.