Add ownerreference for the new created BuildRun about build

[zhangtbj]

I will work on this feature now.

[zhangtbj]

Hi @sbose78 ,

I did some investigations today. I think there are two ways to add the ownerreference for the new created buildRun.

1. Use mutating admission webhook.
But I think, we just need to add ownerreference for buildRun, add a new wekhook deployment and build process just for this ONE ownerreference requirement is too HEAVY now

2, Set the ownerreference in buildRun controller first.
When a new buildRun is created, buildRun controller notice that and add the ownerreference at the beginning of controller reconcile logic.
Such as add:

  ownerReferences:
  - apiVersion: build.dev/v1alpha1
    blockOwnerDeletion: true
    controller: true
    kind: Build
    name: buildpack-nodejs-build
    uid: 9b197859-7bdf-4d58-ac47-f7aea2564bbd

to this buildRun. And I have checked, the generation won't be increased if I add this ownerreference. So we can add it safely in the controller logic and update the ownerreference in the buildRun controller logic.

I prefer to use the second way because it is easy and less dependencies.
But I don't know what we plan to implement in the issue Validate the Build on creation (#83)

Will we introduce the validating webhook to do that? (Also FYI @qu1queee )

If yes, we can also introduce the mutating webhook for the ownerreference together.

If we don't plan to use webhook for validation too. I think we can use option 2.

Any idea, all?

Thanks!

[sbose78]

Stay away from webhooks as much as possible ;)
So, yes, the second one is better.

[zhangtbj]

Agree. Let me try the second way first. :)

[sbose78]

I have a design question though. If I delete the build configuration and all buildruns also get removed, would that lead to loss of trace-ability of complete|successful|failed builds ?

[zhangtbj]

How do you do in the build v1?

If the end-user deletes the BuildConfig, will you also delete the related Builds in build v1 now?

Will those buildRuns to be the orphan resources if we don't delete them together?

[sbose78]

Yes, in Build v1, they were ownerRef'd. Yeah, they will be orphan resources for sure - but I'm wondering if there's a good reason for having them stick around 🤔.

( you can go ahead with the changes - I was thinking out aloud..)

[zhangtbj]

em..... I just had a try on OpenShift v4.x.

Interesting, there is a dialog pop up when you want to delete a BuildConfig:

Delete Build Config
Delete dotnet?

Are you sure you want to delete dotnet in namespace default?
[x] Delete dependent objects of this resource

• If you choose Delete dependent objects of this resource, the Builds will be deleted together.
• If you don't choose Delete dependent objects of this resource, the Builds will be kept

So..... should we add the logic in Build controller?

• If there is any parameter (like a new annotation) from UX to ask to delete dependencies, then query all related resources and delete them?

[sbose78]

Yeah, the annotation path looks good. Maybe we label the buildRuns in a way that they are easy to look up?

[zhangtbj]

• Add annotation path and delete logic in Build controller
• Add label for buildRuns in buildRun controller reconcile at the beginning if it doesn't exist

How about this way? :)

[sbose78]

Add annotation path and delete logic in Build controller

While handling deletion, check if there's an annotation that says "don't delete BuildRuns. If present, then skip deletion of BuildRuns.

Add label for buildRuns in buildRun controller reconcile at the beginning if it doesn't exist

Helps us query, yes.

Is that what you had in mind?

[zhangtbj]

yes, that is my point.

[sbose78]

Sounds good, Jordan!

[qu1queee]

this is done , closing