Skip to content

Commit

Permalink
refactor(pullrequest): move from annotations to dedicated fields in S…
Browse files Browse the repository at this point in the history 
…tatus (#192)

* refactor(pullrequest): move from annotations to dedicated fields in Status

* lint(pullrequest): error not checked
  • Loading branch information
@Alan-pad
Alan-pad authored Nov 13, 2023
1 parent b8f373f commit 62639e1
Show file tree
Hide file tree
Showing 9 changed files with 169 additions and 121 deletions.
6 changes: 4 additions & 2 deletions api/v1alpha1/terraformpullrequest_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,10 @@ type TerraformPullRequestSpec struct {

// TerraformPullRequestStatus defines the observed state of TerraformPullRequest
type TerraformPullRequestStatus struct {
Conditions []metav1.Condition `json:"conditions,omitempty"`
State string `json:"state,omitempty"`
Conditions []metav1.Condition `json:"conditions,omitempty"`
State string `json:"state,omitempty"`
LastDiscoveredCommit string `json:"lastDiscoveredCommit,omitempty"`
LastCommentedCommit string `json:"lastCommentedCommit,omitempty"`
}

// +kubebuilder:object:root=true
Expand Down
6 changes: 1 addition & 5 deletions internal/annotations/annotations.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,7 @@ const (
LastBranchCommit string = "webhook.terraform.padok.cloud/branch-commit"
LastRelevantCommit string = "webhook.terraform.padok.cloud/relevant-commit"

ForceApply string = "notifications.terraform.padok.cloud/force-apply"

LastDiscoveredCommit string = "pullrequest.terraform.padok.cloud/last-discovered-commit"
LastCommentedCommit string = "pullrequest.terraform.padok.cloud/last-commented-commit"

ForceApply string = "notifications.terraform.padok.cloud/force-apply"
AdditionnalTriggerPaths string = "config.terraform.padok.cloud/additionnal-trigger-paths"
)

Expand Down
20 changes: 10 additions & 10 deletions internal/controllers/terraformpullrequest/conditions.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ func (r *Reconciler) IsLastCommitDiscovered(pr *configv1alpha1.TerraformPullRequ
Status: metav1.ConditionUnknown,
LastTransitionTime: metav1.NewTime(time.Now()),
}
lastDiscoveredCommit, ok := pr.Annotations[annotations.LastDiscoveredCommit]
if !ok {
lastDiscoveredCommit := pr.Status.LastDiscoveredCommit
if lastDiscoveredCommit == "" {
condition.Reason = "NoCommitDiscovered"
condition.Message = "Controller hasn't discovered any commit yet."
condition.Status = metav1.ConditionFalse
Expand Down Expand Up @@ -50,7 +50,7 @@ func (r *Reconciler) AreLayersStillPlanning(pr *configv1alpha1.TerraformPullRequ
}
layers, err := GetLinkedLayers(r.Client, pr)

lastDiscoveredCommit, okDiscoveredCommit := pr.Annotations[annotations.LastDiscoveredCommit]
lastDiscoveredCommit := pr.Status.LastDiscoveredCommit
prLastBranchCommit, okPRBranchCommit := pr.Annotations[annotations.LastBranchCommit]

if !okPRBranchCommit {
Expand All @@ -60,7 +60,7 @@ func (r *Reconciler) AreLayersStillPlanning(pr *configv1alpha1.TerraformPullRequ
return condition, true
}

if !okDiscoveredCommit {
if lastDiscoveredCommit == "" {
condition.Reason = "NoCommitDiscovered"
condition.Message = "Controller hasn't discovered any commit yet."
condition.Status = metav1.ConditionTrue
Expand Down Expand Up @@ -114,15 +114,15 @@ func (r *Reconciler) IsCommentUpToDate(pr *configv1alpha1.TerraformPullRequest)
Status: metav1.ConditionUnknown,
LastTransitionTime: metav1.NewTime(time.Now()),
}
lastDiscoveredCommit, ok := pr.Annotations[annotations.LastDiscoveredCommit]
if !ok {
condition.Reason = "Unknown"
condition.Message = "This should not have happened"
lastDiscoveredCommit := pr.Status.LastDiscoveredCommit
if lastDiscoveredCommit == "" {
condition.Reason = "UnDiscovered"
condition.Message = "Pull request has not been discovered yet."
condition.Status = metav1.ConditionUnknown
return condition, true
}
lastCommentedCommit, ok := pr.Annotations[annotations.LastCommentedCommit]
if !ok {
lastCommentedCommit := pr.Status.LastCommentedCommit
if lastCommentedCommit == "" {
condition.Reason = "NoCommentSent"
condition.Message = "No comment has ever been sent"
condition.Status = metav1.ConditionFalse
Expand Down
7 changes: 3 additions & 4 deletions internal/controllers/terraformpullrequest/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,10 +81,9 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
log.Errorf("failed to get TerraformRepository: %s", err)
return ctrl.Result{}, err
}
state, conditions := r.GetState(ctx, pr)
result := state.getHandler()(ctx, r, repository, pr)

pr.Status = configv1alpha1.TerraformPullRequestStatus{Conditions: conditions, State: getStateString(state)}
state := r.GetState(ctx, pr)
result := state.Handler(ctx, r, repository, pr)
pr.Status = state.Status
err = r.Client.Status().Update(ctx, pr)
if err != nil {
log.Errorf("could not update pull request %s status: %s", pr.Name, err)
Expand Down
79 changes: 66 additions & 13 deletions internal/controllers/terraformpullrequest/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,27 @@ var _ = BeforeSuite(func() {
//+kubebuilder:scaffold:scheme

k8sClient, err = client.New(cfg, client.Options{Scheme: scheme.Scheme})
Expect(err).NotTo(HaveOccurred())
utils.LoadResources(k8sClient, "testdata")
statuses := []StatusUpdate{
{
Name: "pr-nominal-case-3",
Namespace: "default",
Status: configv1alpha1.TerraformPullRequestStatus{
LastDiscoveredCommit: "04410b5b7d90b82ad658b86564a9aa4bce411ac9",
LastCommentedCommit: "04410b5b7d90b82ad658b86564a9aa4bce411ac9",
},
},
{
Name: "pr-nominal-case-2",
Namespace: "default",
Status: configv1alpha1.TerraformPullRequestStatus{
LastDiscoveredCommit: "04410b5b7d90b82ad658b86564a9aa4bce411ac9",
},
},
}
err = initStatus(k8sClient, statuses)
Expect(err).NotTo(HaveOccurred())
reconciler = &controller.Reconciler{
Client: k8sClient,
Config: config.TestConfig(),
Expand All @@ -93,6 +113,39 @@ var _ = BeforeSuite(func() {
Expect(k8sClient).NotTo(BeNil())
})

type StatusUpdate struct {
Name string
Namespace string
Status configv1alpha1.TerraformPullRequestStatus
}

func updateStatus(c client.Client, name string, namespace string, status configv1alpha1.TerraformPullRequestStatus) error {
pr := &configv1alpha1.TerraformPullRequest{}
err := c.Get(context.Background(), types.NamespacedName{
Name: name,
Namespace: namespace,
}, pr)
if err != nil {
return err
}
pr.Status = status
err = c.Status().Update(context.Background(), pr)
if err != nil {
return err
}
return nil
}

func initStatus(c client.Client, statuses []StatusUpdate) error {
for _, status := range statuses {
err := updateStatus(c, status.Name, status.Namespace, status.Status)
if err != nil {
return err
}
}
return nil
}

func getResult(name types.NamespacedName) (reconcile.Result, *configv1alpha1.TerraformPullRequest, error, error) {
result, reconcileError := reconciler.Reconcile(context.TODO(), reconcile.Request{
NamespacedName: name,
Expand Down Expand Up @@ -138,7 +191,7 @@ var _ = Describe("TerraformPullRequest controller", func() {
})
Context("Has discovered commit annotation", func() {
It("Should return true", func() {
pr.Annotations[annotations.LastDiscoveredCommit] = "04410b5b7d90b82ad658b86564a9aa4bce411ac9"
pr.Status.LastDiscoveredCommit = "04410b5b7d90b82ad658b86564a9aa4bce411ac9"
_, value := reconciler.IsLastCommitDiscovered(pr)
Expect(value).To(BeTrue())
})
Expand All @@ -161,24 +214,24 @@ var _ = Describe("TerraformPullRequest controller", func() {
})
Context("Has discovered annotation", func() {
It("Should return false", func() {
pr.Annotations[annotations.LastDiscoveredCommit] = "04410b5b7d90b82ad658b86564a9aa4bce411ac9"
pr.Status.LastDiscoveredCommit = "04410b5b7d90b82ad658b86564a9aa4bce411ac9"
_, value := reconciler.IsCommentUpToDate(pr)
Expect(value).To(BeFalse())
})
})
})
Context("Has discovered annotation and commented annotation equals", func() {
It("Should return true", func() {
pr.Annotations[annotations.LastDiscoveredCommit] = "04410b5b7d90b82ad658b86564a9aa4bce411ac9"
pr.Annotations[annotations.LastCommentedCommit] = "04410b5b7d90b82ad658b86564a9aa4bce411ac9"
pr.Status.LastDiscoveredCommit = "04410b5b7d90b82ad658b86564a9aa4bce411ac9"
pr.Status.LastCommentedCommit = "04410b5b7d90b82ad658b86564a9aa4bce411ac9"
_, value := reconciler.IsCommentUpToDate(pr)
Expect(value).To(BeTrue())
})
})
Context("Has discovered annotation and commented annotation different", func() {
It("Should return false", func() {
pr.Annotations[annotations.LastDiscoveredCommit] = "04410b5b7d90b82ad658b86564a9aa4bce411ac9"
pr.Annotations[annotations.LastCommentedCommit] = "old"
pr.Status.LastDiscoveredCommit = "04410b5b7d90b82ad658b86564a9aa4bce411ac9"
pr.Status.LastCommentedCommit = "old"
_, value := reconciler.IsCommentUpToDate(pr)
Expect(value).To(BeFalse())
})
Expand All @@ -187,7 +240,7 @@ var _ = Describe("TerraformPullRequest controller", func() {
Describe("AreLayersStillPlanning", func() {
var layer *configv1alpha1.TerraformLayer
BeforeEach(func() {
pr.Annotations[annotations.LastDiscoveredCommit] = "04410b5b7d90b82ad658b86564a9aa4bce411ac9"
pr.Status.LastDiscoveredCommit = "04410b5b7d90b82ad658b86564a9aa4bce411ac9"
layer = &configv1alpha1.TerraformLayer{
ObjectMeta: metav1.ObjectMeta{
Name: "test-layer",
Expand All @@ -208,7 +261,7 @@ var _ = Describe("TerraformPullRequest controller", func() {
Context("PR annotations", func() {
Context("No discovered commit annotation", func() {
It("Should return true", func() {
delete(pr.Annotations, annotations.LastDiscoveredCommit)
pr.Status.LastDiscoveredCommit = ""
condition, value := reconciler.AreLayersStillPlanning(pr)
Expect(condition.Reason).To(Equal("NoCommitDiscovered"))
Expect(value).To(BeTrue())
Expand All @@ -224,7 +277,7 @@ var _ = Describe("TerraformPullRequest controller", func() {
})
Context("Discovered and Last Branch different", func() {
It("Should return false", func() {
pr.Annotations[annotations.LastDiscoveredCommit] = "other"
pr.Status.LastDiscoveredCommit = "other"
condition, value := reconciler.AreLayersStillPlanning(pr)
Expect(condition.Reason).To(Equal("StillNeedsDiscovery"))
Expect(value).To(BeTrue())
Expand Down Expand Up @@ -301,7 +354,7 @@ var _ = Describe("TerraformPullRequest controller", func() {
Expect(result.RequeueAfter).To(Equal(reconciler.Config.Controller.Timers.WaitAction))
})
It("should have a LastDiscoveredCommit annotation", func() {
Expect(pr.Annotations[annotations.LastDiscoveredCommit]).To(Equal(pr.Annotations[annotations.LastBranchCommit]))
Expect(pr.Status.LastDiscoveredCommit).To(Equal(pr.Annotations[annotations.LastBranchCommit]))
})
It("should have created 2 temp layers", func() {
layers, err := controller.GetLinkedLayers(k8sClient, pr)
Expand Down Expand Up @@ -330,7 +383,7 @@ var _ = Describe("TerraformPullRequest controller", func() {
Expect(result.RequeueAfter).To(Equal(reconciler.Config.Controller.Timers.WaitAction))
})
It("should have a LastCommentedCommit annotation", func() {
Expect(pr.Annotations[annotations.LastCommentedCommit]).To(Equal(pr.Annotations[annotations.LastDiscoveredCommit]))
Expect(pr.Status.LastCommentedCommit).To(Equal(pr.Status.LastDiscoveredCommit))
})
})
Describe("When a TerraformPullRequest has all its comment up to date", Ordered, func() {
Expand Down Expand Up @@ -375,7 +428,7 @@ var _ = Describe("TerraformPullRequest controller", func() {
Expect(result.RequeueAfter).To(Equal(reconciler.Config.Controller.Timers.WaitAction))
})
It("should have a LastDiscoveredCommit annotation", func() {
Expect(pr.Annotations[annotations.LastDiscoveredCommit]).To(Equal(pr.Annotations[annotations.LastBranchCommit]))
Expect(pr.Status.LastDiscoveredCommit).To(Equal(pr.Annotations[annotations.LastBranchCommit]))
})
It("should not have created temp layers", func() {
layers, err := controller.GetLinkedLayers(k8sClient, pr)
Expand Down Expand Up @@ -425,7 +478,7 @@ var _ = Describe("TerraformPullRequest controller", func() {
Expect(pr.Status.State).To(Equal("DiscoveryNeeded"))
})
It("should not have a LastDiscoveredCommit annotation", func() {
Expect(pr.Annotations).NotTo(HaveKey(annotations.LastDiscoveredCommit))
Expect(pr.Status.LastDiscoveredCommit).To(Equal(""))
})
})
})
Expand Down
Loading

0 comments on commit 62639e1

Please sign in to comment.