Skip to content

Commit 3c10a6e

Browse files
author
Guillaume Leccese
committed
feat(controller): post commit status for direct pushes to the base branch
Commits pushed straight to a repository's base branch never go through a TerraformPullRequest, so no plan/apply commit status was ever posted for them. The terraformrun controller now posts one itself when a run's layer isn't pull-request-managed, disambiguated per layer since there's no PR to aggregate multiple layers under a single status context.
1 parent 76077f5 commit 3c10a6e

9 files changed

Lines changed: 264 additions & 0 deletions

File tree

internal/controllers/manager.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,7 @@ func (c *Controllers) Exec() {
167167
Config: c.config,
168168
Datastore: datastoreClient,
169169
K8SLogClient: clientset,
170+
Credentials: credentialStore,
170171
}).SetupWithManager(mgr); err != nil {
171172
log.Fatalf("unable to create run controller: %s", err)
172173
}

internal/controllers/terraformpullrequest/status/status.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,4 +22,9 @@ type CommitStatus struct {
2222
// Commit overrides which commit the status is posted on. When empty,
2323
// providers fall back to the pull request's LastBranchCommit annotation.
2424
Commit string
25+
// Context overrides the default "burrito/<phase>" context/name used to identify the
26+
// status. Needed when several independent statuses target the same commit, e.g. one
27+
// per layer for commits pushed directly to the base branch, where there is no pull
28+
// request to aggregate them under a single context.
29+
Context string
2530
}

internal/controllers/terraformrun/controller.go

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,9 @@ import (
4343

4444
configv1alpha1 "github.com/padok-team/burrito/api/v1alpha1"
4545
"github.com/padok-team/burrito/internal/burrito/config"
46+
repo "github.com/padok-team/burrito/internal/repository"
47+
"github.com/padok-team/burrito/internal/repository/credentials"
48+
repositorytypes "github.com/padok-team/burrito/internal/repository/types"
4649
)
4750

4851
type Clock interface {
@@ -61,9 +64,20 @@ type Reconciler struct {
6164
K8SLogClient *logClient.Clientset
6265
Scheme *runtime.Scheme
6366
Config *config.Config
67+
Credentials *credentials.CredentialStore
6468
Recorder record.EventRecorder
6569
Datastore datastore.Client
6670
Clock
71+
// APIProviderFactory overrides how the API provider is resolved for a repository.
72+
// Only used in tests; production code always uses repository.GetAPIProviderFromRepository.
73+
APIProviderFactory func(repository *configv1alpha1.TerraformRepository) (repositorytypes.APIProvider, error)
74+
}
75+
76+
func (r *Reconciler) getAPIProvider(repository *configv1alpha1.TerraformRepository) (repositorytypes.APIProvider, error) {
77+
if r.APIProviderFactory != nil {
78+
return r.APIProviderFactory(repository)
79+
}
80+
return repo.GetAPIProviderFromRepository(r.Credentials, repository)
6781
}
6882

6983
//+kubebuilder:rbac:groups=config.terraform.padok.cloud,resources=terraformruns,verbs=get;list;watch;create;update;patch;delete

internal/controllers/terraformrun/controller_test.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import (
1515
configv1alpha1 "github.com/padok-team/burrito/api/v1alpha1"
1616
controller "github.com/padok-team/burrito/internal/controllers/terraformrun"
1717
datastore "github.com/padok-team/burrito/internal/datastore/client"
18+
"github.com/padok-team/burrito/internal/repository/credentials"
1819
utils "github.com/padok-team/burrito/internal/testing"
1920
corev1 "k8s.io/api/core/v1"
2021
"k8s.io/apimachinery/pkg/types"
@@ -78,6 +79,7 @@ var _ = BeforeSuite(func() {
7879
Clock: &MockClock{},
7980
Datastore: datastore.NewMockClient(),
8081
K8SLogClient: logClient,
82+
Credentials: credentials.NewCredentialStore(k8sClient, config.TestConfig().Controller.Timers.CredentialsTTL),
8183
Recorder: record.NewBroadcasterForTests(1*time.Second).NewRecorder(scheme.Scheme, corev1.EventSource{
8284
Component: "burrito",
8385
}),
@@ -93,6 +95,7 @@ var _ = BeforeSuite(func() {
9395
Clock: &MockClock{},
9496
Datastore: datastore.NewMockClient(),
9597
K8SLogClient: logClient,
98+
Credentials: credentials.NewCredentialStore(k8sClient, configMaxConcurrent.Controller.Timers.CredentialsTTL),
9699
Recorder: record.NewBroadcasterForTests(1*time.Second).NewRecorder(scheme.Scheme, corev1.EventSource{
97100
Component: "burrito",
98101
}),

internal/controllers/terraformrun/states.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88

99
configv1alpha1 "github.com/padok-team/burrito/api/v1alpha1"
1010
"github.com/padok-team/burrito/internal/controllers/metrics"
11+
"github.com/padok-team/burrito/internal/controllers/terraformpullrequest/status"
1112
"github.com/padok-team/burrito/internal/lock"
1213
corev1 "k8s.io/api/core/v1"
1314
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -190,6 +191,8 @@ func (s *Succeeded) getHandler() Handler {
190191
return ctrl.Result{RequeueAfter: r.Config.Controller.Timers.OnError}, getRunInfo(run)
191192
}
192193

194+
r.setCommitStatusForDirectPush(ctx, run, layer, repo, status.StateSuccess)
195+
193196
metrics.RecordRunCompleted(*run)
194197

195198
return ctrl.Result{RequeueAfter: r.Config.Controller.Timers.WaitAction}, getRunInfo(run)
@@ -209,6 +212,8 @@ func (s *Failed) getHandler() Handler {
209212
return ctrl.Result{RequeueAfter: r.Config.Controller.Timers.OnError}, getRunInfo(run)
210213
}
211214

215+
r.setCommitStatusForDirectPush(ctx, run, layer, repo, status.StateFailure)
216+
212217
metrics.RecordRunFailed(*run)
213218

214219
return ctrl.Result{RequeueAfter: r.Config.Controller.Timers.WaitAction}, getRunInfo(run)
Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
package terraformrun
2+
3+
import (
4+
"context"
5+
"fmt"
6+
7+
configv1alpha1 "github.com/padok-team/burrito/api/v1alpha1"
8+
"github.com/padok-team/burrito/internal/controllers/terraformpullrequest/status"
9+
logrus "github.com/sirupsen/logrus"
10+
)
11+
12+
// isPullRequestLayer reports whether layer was generated for an open pull/merge request.
13+
// Those already get an aggregated plan/apply status posted by the terraformpullrequest
14+
// controller once all their layers are done; posting one here too would just clobber it.
15+
func isPullRequestLayer(layer *configv1alpha1.TerraformLayer) bool {
16+
for _, ref := range layer.OwnerReferences {
17+
if ref.Kind == "TerraformPullRequest" {
18+
return true
19+
}
20+
}
21+
return false
22+
}
23+
24+
// setCommitStatusForDirectPush posts a plan/apply commit status for a run against a layer
25+
// that tracks the repository's base branch directly. Commits pushed straight to the base
26+
// branch never go through a TerraformPullRequest, so nothing else posts a status for them.
27+
func (r *Reconciler) setCommitStatusForDirectPush(ctx context.Context, run *configv1alpha1.TerraformRun, layer *configv1alpha1.TerraformLayer, repository *configv1alpha1.TerraformRepository, state status.State) {
28+
if isPullRequestLayer(layer) || run.Spec.Layer.Revision == "" {
29+
return
30+
}
31+
32+
provider, err := r.getAPIProvider(repository)
33+
if err != nil {
34+
logrus.Warnf("could not get API provider to set commit status for run %s: %s", run.Name, err)
35+
return
36+
}
37+
38+
phase := status.PhasePlan
39+
if run.Spec.Action == string(ApplyAction) {
40+
phase = status.PhaseApply
41+
}
42+
description := fmt.Sprintf("Burrito %s succeeded", phase)
43+
if state == status.StateFailure {
44+
description = fmt.Sprintf("Burrito %s failed", phase)
45+
}
46+
47+
s := status.CommitStatus{
48+
Phase: phase,
49+
State: state,
50+
Description: description,
51+
Commit: run.Spec.Layer.Revision,
52+
// Several layers can target the same commit on a direct push, with no pull
53+
// request to aggregate them under a single context: disambiguate by layer.
54+
Context: fmt.Sprintf("burrito/%s/%s", phase, layer.Name),
55+
}
56+
if err := provider.SetStatus(repository, nil, s); err != nil {
57+
logrus.Warnf("could not set %s commit status for run %s: %s", phase, run.Name, err)
58+
}
59+
}
Lines changed: 171 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,171 @@
1+
package terraformrun
2+
3+
import (
4+
"context"
5+
"errors"
6+
"testing"
7+
8+
configv1alpha1 "github.com/padok-team/burrito/api/v1alpha1"
9+
"github.com/padok-team/burrito/internal/controllers/terraformpullrequest/comment"
10+
"github.com/padok-team/burrito/internal/controllers/terraformpullrequest/status"
11+
repositorytypes "github.com/padok-team/burrito/internal/repository/types"
12+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
13+
)
14+
15+
type fakeAPIProvider struct {
16+
setStatusCalls []status.CommitStatus
17+
setStatusErr error
18+
getProviderErr error
19+
}
20+
21+
func (p *fakeAPIProvider) GetChanges(repository *configv1alpha1.TerraformRepository, pullRequest *configv1alpha1.TerraformPullRequest) ([]string, error) {
22+
return nil, nil
23+
}
24+
25+
func (p *fakeAPIProvider) Comment(repository *configv1alpha1.TerraformRepository, pullRequest *configv1alpha1.TerraformPullRequest, c comment.Comment) error {
26+
return nil
27+
}
28+
29+
func (p *fakeAPIProvider) ListPullRequests(repository *configv1alpha1.TerraformRepository) ([]configv1alpha1.TerraformPullRequest, error) {
30+
return nil, nil
31+
}
32+
33+
func (p *fakeAPIProvider) SetStatus(repository *configv1alpha1.TerraformRepository, pullRequest *configv1alpha1.TerraformPullRequest, s status.CommitStatus) error {
34+
p.setStatusCalls = append(p.setStatusCalls, s)
35+
return p.setStatusErr
36+
}
37+
38+
func (p *fakeAPIProvider) GetMergeCommit(repository *configv1alpha1.TerraformRepository, pullRequest *configv1alpha1.TerraformPullRequest) (string, error) {
39+
return "", nil
40+
}
41+
42+
func testRepository() *configv1alpha1.TerraformRepository {
43+
return &configv1alpha1.TerraformRepository{
44+
ObjectMeta: metav1.ObjectMeta{Name: "repo", Namespace: "default"},
45+
}
46+
}
47+
48+
func testMainLayer() *configv1alpha1.TerraformLayer {
49+
return &configv1alpha1.TerraformLayer{
50+
ObjectMeta: metav1.ObjectMeta{Name: "pwet", Namespace: "default"},
51+
}
52+
}
53+
54+
func testPullRequestLayer() *configv1alpha1.TerraformLayer {
55+
return &configv1alpha1.TerraformLayer{
56+
ObjectMeta: metav1.ObjectMeta{
57+
Name: "pwet-pr-abcdef",
58+
Namespace: "default",
59+
OwnerReferences: []metav1.OwnerReference{
60+
{Kind: "TerraformPullRequest", Name: "repo-1"},
61+
},
62+
},
63+
}
64+
}
65+
66+
func testRun(action string, revision string) *configv1alpha1.TerraformRun {
67+
return &configv1alpha1.TerraformRun{
68+
ObjectMeta: metav1.ObjectMeta{Name: "pwet-" + action + "-abcde", Namespace: "default"},
69+
Spec: configv1alpha1.TerraformRunSpec{
70+
Action: action,
71+
Layer: configv1alpha1.TerraformRunLayer{Revision: revision},
72+
},
73+
}
74+
}
75+
76+
func TestIsPullRequestLayer(t *testing.T) {
77+
if isPullRequestLayer(testMainLayer()) {
78+
t.Fatalf("expected a plain main-branch layer to not be considered a pull request layer")
79+
}
80+
if !isPullRequestLayer(testPullRequestLayer()) {
81+
t.Fatalf("expected a layer owned by a TerraformPullRequest to be considered a pull request layer")
82+
}
83+
}
84+
85+
func TestSetCommitStatusForDirectPushSkipsPullRequestLayers(t *testing.T) {
86+
provider := &fakeAPIProvider{}
87+
r := &Reconciler{
88+
APIProviderFactory: func(repository *configv1alpha1.TerraformRepository) (repositorytypes.APIProvider, error) {
89+
return provider, nil
90+
},
91+
}
92+
r.setCommitStatusForDirectPush(context.Background(), testRun("plan", "sha123"), testPullRequestLayer(), testRepository(), status.StateSuccess)
93+
if len(provider.setStatusCalls) != 0 {
94+
t.Fatalf("expected no commit status to be set for a pull request layer, got %d calls", len(provider.setStatusCalls))
95+
}
96+
}
97+
98+
func TestSetCommitStatusForDirectPushSkipsWhenRevisionIsEmpty(t *testing.T) {
99+
provider := &fakeAPIProvider{}
100+
r := &Reconciler{
101+
APIProviderFactory: func(repository *configv1alpha1.TerraformRepository) (repositorytypes.APIProvider, error) {
102+
return provider, nil
103+
},
104+
}
105+
r.setCommitStatusForDirectPush(context.Background(), testRun("plan", ""), testMainLayer(), testRepository(), status.StateSuccess)
106+
if len(provider.setStatusCalls) != 0 {
107+
t.Fatalf("expected no commit status to be set when the run has no revision, got %d calls", len(provider.setStatusCalls))
108+
}
109+
}
110+
111+
func TestSetCommitStatusForDirectPushPostsPlanStatus(t *testing.T) {
112+
provider := &fakeAPIProvider{}
113+
r := &Reconciler{
114+
APIProviderFactory: func(repository *configv1alpha1.TerraformRepository) (repositorytypes.APIProvider, error) {
115+
return provider, nil
116+
},
117+
}
118+
layer := testMainLayer()
119+
r.setCommitStatusForDirectPush(context.Background(), testRun("plan", "sha123"), layer, testRepository(), status.StateSuccess)
120+
121+
if len(provider.setStatusCalls) != 1 {
122+
t.Fatalf("expected exactly one commit status to be set, got %d", len(provider.setStatusCalls))
123+
}
124+
got := provider.setStatusCalls[0]
125+
if got.Phase != status.PhasePlan {
126+
t.Errorf("expected phase %q, got %q", status.PhasePlan, got.Phase)
127+
}
128+
if got.State != status.StateSuccess {
129+
t.Errorf("expected state %q, got %q", status.StateSuccess, got.State)
130+
}
131+
if got.Commit != "sha123" {
132+
t.Errorf("expected commit %q, got %q", "sha123", got.Commit)
133+
}
134+
wantContext := "burrito/plan/pwet"
135+
if got.Context != wantContext {
136+
t.Errorf("expected context %q, got %q", wantContext, got.Context)
137+
}
138+
}
139+
140+
func TestSetCommitStatusForDirectPushPostsApplyFailureStatus(t *testing.T) {
141+
provider := &fakeAPIProvider{}
142+
r := &Reconciler{
143+
APIProviderFactory: func(repository *configv1alpha1.TerraformRepository) (repositorytypes.APIProvider, error) {
144+
return provider, nil
145+
},
146+
}
147+
r.setCommitStatusForDirectPush(context.Background(), testRun("apply", "sha456"), testMainLayer(), testRepository(), status.StateFailure)
148+
149+
if len(provider.setStatusCalls) != 1 {
150+
t.Fatalf("expected exactly one commit status to be set, got %d", len(provider.setStatusCalls))
151+
}
152+
got := provider.setStatusCalls[0]
153+
if got.Phase != status.PhaseApply {
154+
t.Errorf("expected phase %q, got %q", status.PhaseApply, got.Phase)
155+
}
156+
if got.State != status.StateFailure {
157+
t.Errorf("expected state %q, got %q", status.StateFailure, got.State)
158+
}
159+
if got.Description != "Burrito apply failed" {
160+
t.Errorf("unexpected description: %q", got.Description)
161+
}
162+
}
163+
164+
func TestSetCommitStatusForDirectPushDoesNotPanicOnProviderError(t *testing.T) {
165+
r := &Reconciler{
166+
APIProviderFactory: func(repository *configv1alpha1.TerraformRepository) (repositorytypes.APIProvider, error) {
167+
return nil, errors.New("no provider configured")
168+
},
169+
}
170+
r.setCommitStatusForDirectPush(context.Background(), testRun("plan", "sha123"), testMainLayer(), testRepository(), status.StateSuccess)
171+
}

internal/repository/providers/github/api.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,9 @@ func (api *APIProvider) SetStatus(repository *configv1alpha1.TerraformRepository
5656
commit = pr.Annotations[annotations.LastBranchCommit]
5757
}
5858
ctx := "burrito/" + string(s.Phase)
59+
if s.Context != "" {
60+
ctx = s.Context
61+
}
5962
state := string(s.State)
6063
description := s.Description
6164
_, _, err := api.client.Repositories.CreateStatus(context.TODO(), owner, repoName, commit, github.RepoStatus{

internal/repository/providers/gitlab/api.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,9 @@ func (api *APIProvider) SetStatus(repository *configv1alpha1.TerraformRepository
5252
commit = pr.Annotations[annotations.LastBranchCommit]
5353
}
5454
name := "burrito/" + string(s.Phase)
55+
if s.Context != "" {
56+
name = s.Context
57+
}
5558
description := s.Description
5659
state := toGitlabBuildState(s.State)
5760
_, _, err := api.client.Commits.SetCommitStatus(getGitlabNamespacedName(repository.Spec.Repository.Url), commit, &gitlab.SetCommitStatusOptions{

0 commit comments

Comments
 (0)