Handle non-existing annotation

[SaschaSchwarze0]

I just noticed these output in the operator logs in a travis build https://travis-ci.org/github/shipwright-io/build/builds/740734332:

{"level":"info","ts":1604290243.6176696,"logger":"build.build-controller","msg":"the annotation build.build.dev/build-run-deletion was not properly defined, supported values are true or false","namespace":"default","name":"buildpacks-v3-ruby-x42q8"}
{"level":"info","ts":1604290243.617835,"logger":"build.build-controller","msg":"unexpected error during ownership reference validation","namespace":"default","name":"buildpacks-v3-ruby-x42q8","error":"the annotation build.build.dev/build-run-deletion was not properly defined, supported values are true or false"}

We forgot to handle the non-existing annotation in the same was as one with value false. I am correcting this.

[qu1queee]

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.

[SaschaSchwarze0]

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.

[qu1queee]

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.

[qu1queee]

Requesting test case for the flow when the annotation gets deleted, but previous ownerreferences still define.

[SaschaSchwarze0]

/test 4.4-unit

[SaschaSchwarze0]

Requesting test case for the flow when the annotation gets deleted, but previous ownerreferences still define.

Integration test added.

[qu1queee]

Thanks, lgtm.

[qu1queee]

/approve

[zhangtbj]

/lgtm

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qu1queee, zhangtbj

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [qu1queee,zhangtbj]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment