Resolved verification issues and resolve review comments.

Signed-off-by: Raghav Bhatnagar <raghavbhatnagar96@gmail.com>
Signed-off-by: Pravallika Dindukurthi <dindukurthi.pravallika@gmail.com>

deploy/crds/shipwright.io_buildruns.yaml

@@ -366,13 +366,13 @@ spec:
                         minimum: 1
                         type: integer
                       ttlAfterFailed:
-                        description: TtlAfterFailed defines the maximum duration of
+                        description: TTLAfterFailed defines the maximum duration of
                           time the failed buildrun should exist.
                         format: duration
                         type: string
                       ttlAfterSucceeded:
-                        description: TtlAfterFailed defines the maximum duration of
-                          time the succeeded buildrun should exist.
+                        description: TTLAfterSucceeded defines the maximum duration
+                          of time the succeeded buildrun should exist.
                         format: duration
                         type: string
                     type: object
@@ -710,6 +710,20 @@ spec:
                   - name
                   type: object
                 type: array
+              retention:
+                description: Contains information about retention params
+                properties:
+                  ttlAfterFailed:
+                    description: TTLAfterFailed defines the maximum duration of time
+                      the failed buildrun should exist.
+                    format: duration
+                    type: string
+                  ttlAfterSucceeded:
+                    description: TTLAfterSucceeded defines the maximum duration of
+                      time the succeeded buildrun should exist.
+                    format: duration
+                    type: string
+                type: object
               serviceAccount:
                 description: ServiceAccount refers to the kubernetes serviceaccount
                   which is used for resource control. Default serviceaccount will
@@ -1044,6 +1058,30 @@ spec:
                       - name
                       type: object
                     type: array
+                  retention:
+                    description: Contains information about retention params
+                    properties:
+                      failedLimit:
+                        description: FailedLimit defines the maximum number of failed
+                          buildruns that should exist.
+                        minimum: 1
+                        type: integer
+                      succeededLimit:
+                        description: SucceededLimit defines the maximum number of
+                          succeeded buildruns that should exist.
+                        minimum: 1
+                        type: integer
+                      ttlAfterFailed:
+                        description: TTLAfterFailed defines the maximum duration of
+                          time the failed buildrun should exist.
+                        format: duration
+                        type: string
+                      ttlAfterSucceeded:
+                        description: TTLAfterSucceeded defines the maximum duration
+                          of time the succeeded buildrun should exist.
+                        format: duration
+                        type: string
+                    type: object
                   source:
                     description: Source refers to the Git repository containing the
                       source code to be built.

deploy/crds/shipwright.io_builds.yaml

@@ -345,13 +345,13 @@ spec:
                     minimum: 1
                     type: integer
                   ttlAfterFailed:
-                    description: TtlAfterFailed defines the maximum duration of time
+                    description: TTLAfterFailed defines the maximum duration of time
                       the failed buildrun should exist.
                     format: duration
                     type: string
                   ttlAfterSucceeded:
-                    description: TtlAfterFailed defines the maximum duration of time
-                      the succeeded buildrun should exist.
+                    description: TTLAfterSucceeded defines the maximum duration of
+                      time the succeeded buildrun should exist.
                     format: duration
                     type: string
                 type: object

docs/build.md

@@ -569,20 +569,7 @@ An example of a user using both TTL and Limit retention fields. In case of such
       succeededLimit: 20
 ```
 
-**NOTE**: When changes are made to `retention.failedLimit` and `retention.succeededLimit` values, they come into effect as soon as the build is applied, thereby enforcing the new limits. On the other hand, changing the `retention.ttlAfterFailed` and `retention.ttlAfterSucceeded` values will only affect new buildruns. Old buildruns will adhere to the old TTL retention values.
-
-#### Build_limit_cleanup controller
-
-Builds are watched by the build_limit_cleanup controller for the following preconditions:
-
-- Update on `Build` resource if either `retention.failedLimit` or `retention.succeeded` are set
-- Create on `Build` resource if either `retention.failedLimit` or `retention.succeeded` are set
-
-build_limit_cleanup also watches buildruns for the following precondition:
-
-- Update on `BuildRun` resource if either `retention.failedLimit` or `retention.succeeded` are set and the Buildrun has just completed execution.
-
-If these conditions are met, the reconciler calculates the number of buildruns that exists and deletes the oldest ones till the limit is satisfied.
+**NOTE**: When changes are made to `retention.failedLimit` and `retention.succeededLimit` values, they come into effect as soon as the build is applied, thereby enforcing the new limits. On the other hand, changing the `retention.ttlAfterFailed` and `retention.ttlAfterSucceeded` values will only affect new buildruns. Old buildruns will adhere to the old TTL retention values. In case TTL values are defined in buildrun specifications as well as build specifications, priority will be given to the values defined in the buildrun specifications.
 
 ### Sources
 

docs/buildrun.md

@@ -13,6 +13,7 @@ SPDX-License-Identifier: Apache-2.0
   - [Defining the BuildSpec](#defining-the-buildspec)
   - [Defining ParamValues](#defining-paramvalues)
   - [Defining the ServiceAccount](#defining-the-serviceaccount)
+  - [Defining Retention Parameters](#defining-retention-parameters)
 - [Canceling a `BuildRun`](#canceling-a-buildrun)
 - [Automatic `BuildRun` deletion](#automatic-buildrun-deletion)
 - [Specifying Environment Variables](#specifying-environment-variables)
@@ -167,6 +168,32 @@ You can also use set the `spec.serviceAccount.generate` path to `true`. This wil
 
 _**Note**_: When the service account is not defined, the `BuildRun` will use the `pipeline` service account if it exists in the namespace, and fall back to the `default` service account.
 
+### Defining Retention Parameters
+
+A `Buildrun` resource can specify how long a completed BuildRun can exist. Instead of manually cleaning up old BuildRuns, retention parameters provide an alternate method for cleaning up BuildRuns automatically.
+
+As part of the buildrun retention parameters, we have the following fields:
+
+- `retention.ttlAfterFailed` - Specifies the duration for which a failed buildrun can exist.
+- `retention.ttlAfterSucceeded` - Specifies the duration for which a successful buildrun can exist.
+
+An example of a user using buildrun TTL parameters.
+
+```yaml
+apiVersion: shipwright.io/v1alpha1
+kind: BuildRun
+metadata:
+  name: buidrun-retention-ttl
+spec:
+  buildRef:
+    name: build-retention-ttl
+  retention:
+    ttlAfterFailed: 10m
+    ttlAfterSucceeded: 10m
+```
+
+**NOTE** In case TTL values are defined in buildrun specifications as well as build specifications, priority will be given to the values defined in the buildrun specifications.
+
 ## Canceling a `BuildRun`
 
 To cancel a `BuildRun` that's currently executing, update its status to mark it as canceled.
@@ -187,23 +214,17 @@ spec:
 
 ## Automatic `BuildRun` deletion
 
-We have two controllers that ensure that buildruns can be deleted automatically if required. This is ensured by adding `retention` parameters in build specifications.
+We have two controllers that ensure that buildruns can be deleted automatically if required. This is ensured by adding `retention` parameters in either the build specifications or the buildrun specifications.
 
-- TTL parameters: These are used by the buildrun_ttl_cleanup controller.
+- Buildrun TTL parameters: These are used to make sure that buildruns exist for a fixed duration of time after completiion.
+  - `buildrun.spec.retention.ttlAfterFailed`: The buildrun is deleted if the mentioned duration of time has passed and the buildrun has failed.
+  - `buildrun.spec.retention.ttlAfterSucceeded`: The buildrun is deleted if the mentioned duration of time has passed and the buildrun has succeeded.
+- Build TTL parameters: These are used to make sure that related buildruns exist for a fixed duration of time after completiion.
   - `build.spec.retention.ttlAfterFailed`: The buildrun is deleted if the mentioned duration of time has passed and the buildrun has failed.
   - `build.spec.retention.ttlAfterSucceeded`: The buildrun is deleted if the mentioned duration of time has passed and the buildrun has succeeded.
-- Limit parameters: These are used by the build_limit_cleanup controller.
-  - `build.spec.retention.failedLimit`: This parameter ensures that the number of failed buildruns do not exceed this limit. If the limit is exceeded, the oldest failed buildruns are deleted till the limit is satisfied.
-  - `build.spec.retention.succeededLimit`: This parameter ensures that the number of successful buildruns do not exceed this limit. If the limit is exceeded, the oldest successful buildruns are deleted till the limit is satisfied.
-
-### Buildrun_ttl_cleanup controller
-
-Buildruns are watched by the buildrun_ttl_cleanup controller for the following preconditions:
-
-- Update on `Buildrun` resource if either `ttlAfterSucceeded` or `ttlAfterFailed` are set.
-- Create on `Buildrun` resource if either `ttlAfterSucceeded` or `ttlAfterFailed` are set.
-
-If these conditions are met, the reconciler checks if the ttl retention field is satisfied or not. If it is not, the buildrun is requeued after calculating the remaining time the buildrun is allowed to exist.
+- Build Limit parameters: These are used to make sure that related buildruns exist for a fixed duration of time after completiion.
+  - `build.spec.retention.succeededLimit` - Defines number of succeeded BuildRuns for a Build that can exist.
+  - `build.spec.retention.failedLimit` - Defines number of failed BuildRuns for a Build that can exist.
 
 ## Specifying Environment Variables
 

pkg/apis/build/v1alpha1/build_types.go

@@ -220,7 +220,7 @@ type BuildList struct {
 	Items           []Build `json:"items"`
 }
 
-// Retention struct for buildrun cleanup
+// BuildRetention struct for buildrun cleanup
 type BuildRetention struct {
 	// FailedLimit defines the maximum number of failed buildruns that should exist.
 	//
@@ -232,16 +232,16 @@ type BuildRetention struct {
 	// +optional
 	// +kubebuilder:validation:Minimum=1
 	SucceededLimit *uint `json:"succeededLimit,omitempty"`
-	// TtlAfterFailed defines the maximum duration of time the failed buildrun should exist.
+	// TTLAfterFailed defines the maximum duration of time the failed buildrun should exist.
 	//
 	// +optional
 	// +kubebuilder:validation:Format=duration
-	TtlAfterFailed *metav1.Duration `json:"ttlAfterFailed,omitempty"`
-	// TtlAfterFailed defines the maximum duration of time the succeeded buildrun should exist.
+	TTLAfterFailed *metav1.Duration `json:"ttlAfterFailed,omitempty"`
+	// TTLAfterSucceeded defines the maximum duration of time the succeeded buildrun should exist.
 	//
 	// +optional
 	// +kubebuilder:validation:Format=duration
-	TtlAfterSucceeded *metav1.Duration `json:"ttlAfterSucceeded,omitempty"`
+	TTLAfterSucceeded *metav1.Duration `json:"ttlAfterSucceeded,omitempty"`
 }
 
 func init() {

pkg/apis/build/v1alpha1/buildrun_types.go

@@ -72,6 +72,10 @@ type BuildRunSpec struct {
 	//
 	// +optional
 	Env []corev1.EnvVar `json:"env,omitempty"`
+
+	// Contains information about retention params
+	// +optional
+	Retention *BuildRunRetention `json:"retention,omitempty"`
 }
 
 // BuildRunRequestedState defines the buildrun state the user can provide to override whatever is the current state.
@@ -296,6 +300,20 @@ type Condition struct {
 	Message string `json:"message" description:"human-readable message indicating details about last transition"`
 }
 
+// BuildRunRetention struct for buildrun cleanup
+type BuildRunRetention struct {
+	// TTLAfterFailed defines the maximum duration of time the failed buildrun should exist.
+	//
+	// +optional
+	// +kubebuilder:validation:Format=duration
+	TTLAfterFailed *metav1.Duration `json:"ttlAfterFailed,omitempty"`
+	// TTLAfterSucceeded defines the maximum duration of time the succeeded buildrun should exist.
+	//
+	// +optional
+	// +kubebuilder:validation:Format=duration
+	TTLAfterSucceeded *metav1.Duration `json:"ttlAfterSucceeded,omitempty"`
+}
+
 func init() {
 	SchemeBuilder.Register(&BuildRun{}, &BuildRunList{})
 }

pkg/controller/controller.go

@@ -16,9 +16,9 @@ import (
 	"github.com/shipwright-io/build/pkg/config"
 	"github.com/shipwright-io/build/pkg/ctxlog"
 	"github.com/shipwright-io/build/pkg/reconciler/build"
-	"github.com/shipwright-io/build/pkg/reconciler/build_limit_cleanup"
+	"github.com/shipwright-io/build/pkg/reconciler/buildlimitcleanup"
 	"github.com/shipwright-io/build/pkg/reconciler/buildrun"
-	"github.com/shipwright-io/build/pkg/reconciler/buildrun_ttl_cleanup"
+	"github.com/shipwright-io/build/pkg/reconciler/buildrunttlcleanup"
 	"github.com/shipwright-io/build/pkg/reconciler/buildstrategy"
 	"github.com/shipwright-io/build/pkg/reconciler/clusterbuildstrategy"
 )
@@ -65,11 +65,11 @@ func NewManager(ctx context.Context, config *config.Config, cfg *rest.Config, op
 		return nil, err
 	}
 
-	if err := build_limit_cleanup.Add(ctx, config, mgr); err != nil {
+	if err := buildlimitcleanup.Add(ctx, config, mgr); err != nil {
 		return nil, err
 	}
 
-	if err := buildrun_ttl_cleanup.Add(ctx, config, mgr); err != nil {
+	if err := buildrunttlcleanup.Add(ctx, config, mgr); err != nil {
 		return nil, err
 	}