Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle non-existing annotation #460

Merged
merged 2 commits into from
Nov 5, 2020
Merged

Handle non-existing annotation #460

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e27ebcb
Handle empty non-existing annotation
SaschaSchwarze0 Nov 2, 2020
2039031
Add integration test for annotation removal
SaschaSchwarze0 Nov 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions pkg/controller/build/build_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,18 +339,18 @@ func (r *ReconcileBuild) validateBuildRunOwnerReferences(ctx context.Context, b
if err = r.client.Update(ctx, &buildRun); err != nil {
return err
}
ctxlog.Info(ctx, fmt.Sprintf("succesfully updated BuildRun %s", buildRun.Name), namespace, buildRun.Namespace, name, buildRun.Name)
ctxlog.Info(ctx, fmt.Sprintf("successfully updated BuildRun %s", buildRun.Name), namespace, buildRun.Namespace, name, buildRun.Name)
}
}
case "false":
case "", "false":
Copy link
Contributor

@qu1queee qu1queee Nov 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, just to be very clear, the switch is not covering the scenario when build.AnnotationBuildRunDeletion annotation is not present. I think we should not cover this in this switch/case, mainly because there is no need to run the underlying logic when the annotation is not defined.

I think you can use this https://play.golang.org/p/HklUW5yVfas logic, to validate prior to the switch if the annotation key is defined or not.

Copy link
Member Author

@SaschaSchwarze0 SaschaSchwarze0 Nov 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @qu1queee, I think your proposal will mean the following behavior:

  • Create a build b1 without the annotation
  • Create a buildrun br1 which references b1 -> the buildrun has no owner, that's fine

--

  • Create a build b2 with annotation build.build.dev/build-run-deletion=true
  • Create a buildrun br2 which references b2 -> the buildrun br2 will be owned by the build b2
  • Update the build b2 to delete the annotation build.build.dev/build-run-deletion -> the buildrun br2 still has b2 as owner although b2 does not anymore express that buildrun deletion should happen

Based on that, handling "" in the same way as "false" is in my opinion accurate.

Copy link
Contributor

@qu1queee qu1queee Nov 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, sorry it was not clear from the PR that this was the rational. I think the current code was a strict one, based on #409 (review) . I think this is ideal, but you will need to provide an integration test case for this flow. Use https://github.com/shipwright-io/build/blob/master/test/integration/build_to_buildruns_test.go#L344-L372 as a reference, where the patch part should be a removal of the annotation.

// if the buildRun have an ownerreference to the Build, lets remove it
for _, buildRun := range buildRunList.Items {
if index := r.validateBuildOwnerReference(buildRun.OwnerReferences, b); index != -1 {
buildRun.OwnerReferences = removeOwnerReferenceByIndex(buildRun.OwnerReferences, index)
if err := r.client.Update(ctx, &buildRun); err != nil {
return err
}
ctxlog.Info(ctx, fmt.Sprintf("succesfully updated BuildRun %s", buildRun.Name), namespace, buildRun.Namespace, name, buildRun.Name)
ctxlog.Info(ctx, fmt.Sprintf("successfully updated BuildRun %s", buildRun.Name), namespace, buildRun.Namespace, name, buildRun.Name)
}
}

Expand Down
31 changes: 31 additions & 0 deletions test/integration/build_to_buildruns_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,13 @@ package integration_test

import (
"fmt"
"strings"

. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"

"github.com/shipwright-io/build/pkg/apis/build/v1alpha1"
"github.com/shipwright-io/build/test"
Expand Down Expand Up @@ -369,6 +371,35 @@ var _ = Describe("Integration tests Build and BuildRuns", func() {
Expect(err).To(BeNil())
Expect(ownerReferenceNames(br.OwnerReferences)).ShouldNot(ContainElement(buildObject.Name))

})
It("does not deletes the buildrun if the annotation is removed", func() {

Expect(tb.CreateBuild(buildObject)).To(BeNil())

autoDeleteBuildRun, err := tb.Catalog.LoadBRWithNameAndRef(
BUILDRUN+tb.Namespace,
BUILD+tb.Namespace,
[]byte(test.MinimalBuildRun),
)
Expect(err).To(BeNil())

Expect(tb.CreateBR(autoDeleteBuildRun)).To(BeNil())

_, err = tb.GetBRTillStartTime(autoDeleteBuildRun.Name)
Expect(err).To(BeNil())

// we remove the annotation so automatic delete does not take place, "/" is escaped by "~1" in a JSON pointer
data := []byte(fmt.Sprintf(`[{"op":"remove","path":"/metadata/annotations/%s"}]`, strings.ReplaceAll(v1alpha1.AnnotationBuildRunDeletion, "/", "~1")))
_, err = tb.PatchBuildWithPatchType(BUILD+tb.Namespace, data, types.JSONPatchType)
Expect(err).To(BeNil())

err = tb.DeleteBuild(BUILD + tb.Namespace)
Expect(err).To(BeNil())

br, err := tb.GetBR(BUILDRUN + tb.Namespace)
Expect(err).To(BeNil())
Expect(ownerReferenceNames(br.OwnerReferences)).ShouldNot(ContainElement(buildObject.Name))

})
It("does delete the buildrun after several modifications of the annotation", func() {

Expand Down
9 changes: 7 additions & 2 deletions test/integration/utils/builds.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,15 @@ func (t *TestBuild) GetBuild(name string) (*v1alpha1.Build, error) {
Get(name, metav1.GetOptions{})
}

// PatchBuild patches an existing Build
// PatchBuild patches an existing Build using the merge patch type
func (t *TestBuild) PatchBuild(buildName string, data []byte) (*v1alpha1.Build, error) {
return t.PatchBuildWithPatchType(buildName, data, types.MergePatchType)
}

// PatchBuildWithPatchType patches an existing Build and allows specifying the patch type
func (t *TestBuild) PatchBuildWithPatchType(buildName string, data []byte, pt types.PatchType) (*v1alpha1.Build, error) {
bInterface := t.BuildClientSet.BuildV1alpha1().Builds(t.Namespace)
b, err := bInterface.Patch(buildName, types.MergePatchType, data)
b, err := bInterface.Patch(buildName, pt, data)
if err != nil {
return nil, err
}
Expand Down