fix(pr): multiple bugs in the PR controller (#187)

* fix(push): only inspect layers on affected repos

* fix(pr): add logs when creating PR

* fix(logging): add resource reconciliated

* fix(pr): change name of temporary layers

* fix(pr): delete temporary layers before discovery

* feat(pr): add Planning state

* fix(rbac): add deletecollection verb on layers

* fix(pr-comment): add whitespace for correct headings

deploy/charts/burrito/templates/rbac.yaml

@@ -162,6 +162,7 @@ rules:
   verbs:
   - create
   - delete
+  - deletecollection
   - get
   - list
   - patch

internal/controllers/terraformlayer/controller.go

@@ -70,7 +70,7 @@ type Reconciler struct {
 // - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.13.0/pkg/reconcile
 func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
 	log := log.WithContext(ctx)
-	log.Infof("starting reconciliation...")
+	log.Infof("starting reconciliation for layer %s/%s ...", req.Namespace, req.Name)
 	layer := &configv1alpha1.TerraformLayer{}
 	err := r.Client.Get(ctx, req.NamespacedName, layer)
 	if errors.IsNotFound(err) {
@@ -123,7 +123,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 	if err != nil {
 		log.Errorf("could not update layer %s status: %s", layer.Name, err)
 	}
-	log.Infof("finished reconciliation cycle for layer %s", layer.Name)
+	log.Infof("finished reconciliation cycle for layer %s/%s", layer.Namespace, layer.Name)
 	return result, nil
 }
 

internal/controllers/terraformpullrequest/comment/templates/comment.md

@@ -2,7 +2,8 @@
 
 {{ len .Layers }} layer(s) affected with {{ .Commit }} commit.
 
-{{- range .Layers }}
+{{ range .Layers }}
+
 ### Layer {{ .Path }}
 
 `{{ .ShortDiff }}`
@@ -14,4 +15,5 @@
 {{ .PrettyPlan }}
 ```
 </details>
-{{- end }}
+
+{{ end }}

internal/controllers/terraformpullrequest/conditions.go

@@ -1,15 +1,11 @@
 package terraformpullrequest
 
 import (
-	"context"
 	"time"
 
 	configv1alpha1 "github.com/padok-team/burrito/api/v1alpha1"
 	"github.com/padok-team/burrito/internal/annotations"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/labels"
-	"k8s.io/apimachinery/pkg/selection"
-	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 func (r *Reconciler) IsLastCommitDiscovered(pr *configv1alpha1.TerraformPullRequest) (metav1.Condition, bool) {
@@ -29,7 +25,7 @@ func (r *Reconciler) IsLastCommitDiscovered(pr *configv1alpha1.TerraformPullRequ
 	lastBranchCommit, ok := pr.Annotations[annotations.LastBranchCommit]
 	if !ok {
 		condition.Reason = "UnknownLastBranchCommit"
-		condition.Message = "This should have happened"
+		condition.Message = "This should not have happened"
 		condition.Status = metav1.ConditionFalse
 		return condition, false
 	}
@@ -143,17 +139,3 @@ func (r *Reconciler) IsCommentUpToDate(pr *configv1alpha1.TerraformPullRequest)
 	condition.Status = metav1.ConditionTrue
 	return condition, true
 }
-
-func GetLinkedLayers(cl client.Client, pr *configv1alpha1.TerraformPullRequest) ([]configv1alpha1.TerraformLayer, error) {
-	layers := configv1alpha1.TerraformLayerList{}
-	requirement, err := labels.NewRequirement("burrito/managed-by", selection.Equals, []string{pr.Name})
-	if err != nil {
-		return nil, err
-	}
-	selector := labels.NewSelector().Add(*requirement)
-	err = cl.List(context.TODO(), &layers, client.MatchingLabelsSelector{Selector: selector})
-	if err != nil {
-		return nil, err
-	}
-	return layers.Items, nil
-}

internal/controllers/terraformpullrequest/controller.go

@@ -55,9 +55,8 @@ type Reconciler struct {
 // For more details, check Reconcile and its Result here:
 // - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.13.0/pkg/reconcile
 func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
-
 	log := log.WithContext(ctx)
-	log.Infof("starting reconciliation...")
+	log.Infof("starting reconciliation for pull request %s/%s ...", req.Namespace, req.Name)
 	pr := &configv1alpha1.TerraformPullRequest{}
 	err := r.Client.Get(ctx, req.NamespacedName, pr)
 	if errors.IsNotFound(err) {
@@ -89,7 +88,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 	if err != nil {
 		log.Errorf("could not update pull request %s status: %s", pr.Name, err)
 	}
-	log.Infof("finished reconciliation cycle for pull request %s", pr.Name)
+	log.Infof("finished reconciliation cycle for pull request %s/%s", pr.Namespace, pr.Name)
 	return result, nil
 }
 

internal/controllers/terraformpullrequest/layer.go

@@ -5,10 +5,14 @@ import (
 	"fmt"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/labels"
+	"k8s.io/apimachinery/pkg/selection"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	configv1alpha1 "github.com/padok-team/burrito/api/v1alpha1"
 	"github.com/padok-team/burrito/internal/annotations"
 	controller "github.com/padok-team/burrito/internal/controllers/terraformlayer"
+	log "github.com/sirupsen/logrus"
 )
 
 func (r *Reconciler) getAffectedLayers(repository *configv1alpha1.TerraformRepository, pr *configv1alpha1.TerraformPullRequest) ([]configv1alpha1.TerraformLayer, error) {
@@ -63,7 +67,7 @@ func generateTempLayers(pr *configv1alpha1.TerraformPullRequest, layers []config
 		new := configv1alpha1.TerraformLayer{
 			ObjectMeta: metav1.ObjectMeta{
 				Namespace:    layer.ObjectMeta.Namespace,
-				GenerateName: fmt.Sprintf("%s-%s-", layer.Name, pr.Spec.ID),
+				GenerateName: fmt.Sprintf("%s-pr-%s-", layer.Name, pr.Spec.ID),
 				Annotations: map[string]string{
 					annotations.LastBranchCommit:   pr.Annotations[annotations.LastBranchCommit],
 					annotations.LastRelevantCommit: pr.Annotations[annotations.LastBranchCommit],
@@ -95,3 +99,24 @@ func generateTempLayers(pr *configv1alpha1.TerraformPullRequest, layers []config
 	}
 	return list
 }
+
+func GetLinkedLayers(cl client.Client, pr *configv1alpha1.TerraformPullRequest) ([]configv1alpha1.TerraformLayer, error) {
+	layers := configv1alpha1.TerraformLayerList{}
+	requirement, err := labels.NewRequirement("burrito/managed-by", selection.Equals, []string{pr.Name})
+	if err != nil {
+		return nil, err
+	}
+	selector := labels.NewSelector().Add(*requirement)
+	err = cl.List(context.TODO(), &layers, client.MatchingLabelsSelector{Selector: selector})
+	if err != nil {
+		return nil, err
+	}
+	return layers.Items, nil
+}
+
+func (r *Reconciler) deleteTempLayers(ctx context.Context, pr *configv1alpha1.TerraformPullRequest) error {
+	log.Infof("deleting temporary layers for pull request %s/%s", pr.Namespace, pr.Name)
+	return r.Client.DeleteAllOf(
+		ctx, &configv1alpha1.TerraformLayer{}, client.InNamespace(pr.Namespace), client.MatchingLabels{"burrito/managed-by": pr.Name},
+	)
+}

internal/controllers/terraformpullrequest/states.go

@@ -9,7 +9,6 @@ import (
 	"github.com/padok-team/burrito/internal/annotations"
 	"github.com/padok-team/burrito/internal/controllers/terraformpullrequest/comment"
 	log "github.com/sirupsen/logrus"
-	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	ctrl "sigs.k8s.io/controller-runtime"
 )
@@ -33,7 +32,7 @@ func (r *Reconciler) GetState(ctx context.Context, pr *configv1alpha1.TerraformP
 		return &Idle{}, conditions
 	case isLastCommitDiscovered && areLayersStillPlanning:
 		log.Infof("pull request %s layers are still planning, waiting", pr.Name)
-		return &Idle{}, conditions
+		return &Planning{}, conditions
 	case isLastCommitDiscovered && !areLayersStillPlanning && !isCommentUpToDate:
 		log.Infof("pull request %s layers have finished, posting comment", pr.Name)
 		return &CommentNeeded{}, conditions
@@ -51,10 +50,23 @@ func (s *Idle) getHandler() func(ctx context.Context, r *Reconciler, repository
 	}
 }
 
+type Planning struct{}
+
+func (s *Planning) getHandler() func(ctx context.Context, r *Reconciler, repository *configv1alpha1.TerraformRepository, pr *configv1alpha1.TerraformPullRequest) ctrl.Result {
+	return func(ctx context.Context, r *Reconciler, repository *configv1alpha1.TerraformRepository, pr *configv1alpha1.TerraformPullRequest) ctrl.Result {
+		return ctrl.Result{RequeueAfter: r.Config.Controller.Timers.WaitAction}
+	}
+}
+
 type DiscoveryNeeded struct{}
 
 func (s *DiscoveryNeeded) getHandler() func(ctx context.Context, r *Reconciler, repository *configv1alpha1.TerraformRepository, pr *configv1alpha1.TerraformPullRequest) ctrl.Result {
 	return func(ctx context.Context, r *Reconciler, repository *configv1alpha1.TerraformRepository, pr *configv1alpha1.TerraformPullRequest) ctrl.Result {
+		// Delete already existing temporary layers
+		err := r.deleteTempLayers(ctx, pr)
+		if err != nil {
+			log.Errorf("failed to delete temp layers for pull request %s: %s", pr.Name, err)
+		}
 		layers, err := r.getAffectedLayers(repository, pr)
 		if err != nil {
 			log.Errorf("failed to get affected layers for pull request %s: %s", pr.Name, err)
@@ -63,19 +75,12 @@ func (s *DiscoveryNeeded) getHandler() func(ctx context.Context, r *Reconciler,
 		newLayers := generateTempLayers(pr, layers)
 		for _, layer := range newLayers {
 			err := r.Client.Create(ctx, &layer)
-			if errors.IsAlreadyExists(err) {
-				log.Infof("layer %s already exists, updating it", layer.Name)
-				err = r.Client.Update(ctx, &layer)
-				if err != nil {
-					log.Errorf("failed to update layer %s: %s", layer.Name, err)
-					return ctrl.Result{RequeueAfter: r.Config.Controller.Timers.OnError}
-				}
-			}
 			if err != nil {
 				log.Errorf("failed to create layer %s: %s", layer.Name, err)
 				return ctrl.Result{RequeueAfter: r.Config.Controller.Timers.OnError}
 			}
 		}
+		// TODO: setting this annotation triggers another reconciliation cycle while this one is still running, which is not ideal
 		err = annotations.Add(ctx, r.Client, pr, map[string]string{annotations.LastDiscoveredCommit: pr.Annotations[annotations.LastBranchCommit]})
 		if err != nil {
 			log.Errorf("failed to add annotation %s to pull request %s: %s", annotations.LastDiscoveredCommit, pr.Name, err)

internal/controllers/terraformrun/controller.go

@@ -67,7 +67,7 @@ type Reconciler struct {
 // - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.13.0/pkg/reconcile
 func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
 	log := log.WithContext(ctx)
-	log.Infof("starting reconciliation...")
+	log.Infof("starting reconciliation for run %s/%s ...", req.Namespace, req.Name)
 	run := &configv1alpha1.TerraformRun{}
 	err := r.Client.Get(ctx, req.NamespacedName, run)
 	if errors.IsNotFound(err) {
@@ -103,7 +103,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 	if err != nil {
 		log.Errorf("could not update run %s status: %s", run.Name, err)
 	}
-	log.Infof("finished reconciliation cycle for run %s", run.Name)
+	log.Infof("finished reconciliation cycle for run %s/%s", run.Namespace, run.Name)
 	return result, nil
 }
 

internal/webhook/event/pullrequest.go

@@ -51,6 +51,7 @@ func (e *PullRequestEvent) Handle(c client.Client) error {
 func batchCreatePullRequests(ctx context.Context, c client.Client, prs []configv1alpha1.TerraformPullRequest) error {
 	var errResult error
 	for _, pr := range prs {
+		log.Infof("creating terraform pull request %s/%s", pr.Namespace, pr.Name)
 		err := c.Create(ctx, &pr)
 		if err != nil {
 			errResult = multierror.Append(errResult, err)
@@ -62,7 +63,7 @@ func batchCreatePullRequests(ctx context.Context, c client.Client, prs []configv
 func batchDeletePullRequests(ctx context.Context, c client.Client, prs []configv1alpha1.TerraformPullRequest) error {
 	var errResult error
 	for _, pr := range prs {
-		log.Info(fmt.Sprintf("deleting pull request %s", pr.Name))
+		log.Infof("deleting terraform pull request %s/%s", pr.Namespace, pr.Name)
 		err := c.Delete(ctx, &pr)
 		if err != nil {
 			errResult = multierror.Append(errResult, err)

internal/webhook/event/push.go

@@ -47,7 +47,7 @@ func (e *PushEvent) Handle(c client.Client) error {
 		}
 	}
 
-	for _, layer := range e.getAffectedLayers(layers.Items, repositories.Items) {
+	for _, layer := range e.getAffectedLayers(layers.Items, affectedRepositories) {
 		ann := map[string]string{}
 		log.Printf("evaluating terraform layer %s for revision %s", layer.Name, e.Revision)
 		if layer.Spec.Branch != e.Revision {
@@ -57,6 +57,7 @@ func (e *PushEvent) Handle(c client.Client) error {
 		ann[annotations.LastBranchCommit] = e.ChangeInfo.ShaAfter
 
 		if controller.LayerFilesHaveChanged(layer, e.Changes) {
+			log.Infof("layer %s is affected by push event", layer.Name)
 			ann[annotations.LastRelevantCommit] = e.ChangeInfo.ShaAfter
 		}
 
@@ -81,8 +82,10 @@ func (e *PushEvent) Handle(c client.Client) error {
 
 func (e *PushEvent) getAffectedRepositories(repositories []configv1alpha1.TerraformRepository) []configv1alpha1.TerraformRepository {
 	affectedRepositories := []configv1alpha1.TerraformRepository{}
+	log.Infof("looking for affected repositories, event url: %s", e.URL)
 	for _, repo := range repositories {
 		if e.URL == NormalizeUrl(repo.Spec.Repository.Url) {
+			log.Infof("repository %s is affected by push event", repo.Name)
 			affectedRepositories = append(affectedRepositories, repo)
 			continue
 		}