Skip to content

Commit caa8246

Browse files
belitrebelitre
authored
fix: avoid crash if empty settings (#5)
1 parent c667db9 commit caa8246

File tree

8 files changed

+105
-33
lines changed

8 files changed

+105
-33
lines changed

‎go.mod

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ require (
1111
github.com/fsnotify/fsnotify v1.4.9 // indirect
1212
github.com/shurcooL/githubv4 v0.0.0-20200627185320-e003124d66e4
1313
github.com/sirupsen/logrus v1.6.0
14-
github.com/stretchr/testify v1.6.1 // indirect
14+
github.com/stretchr/testify v1.6.1
1515
golang.org/x/exp v0.0.0-20191030013958-a1ab85dbe136 // indirect
1616
golang.org/x/mod v0.3.0 // indirect
1717
golang.org/x/sys v0.0.0-20200625212154-ddb9806d33ae // indirect

‎pkg/config/config.go

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -78,13 +78,17 @@ func (pca *PluginConfigAgent) Load(path string) error {
7878
// Set sets the plugin agent configuration.
7979
func (pca *PluginConfigAgent) Set(pc *Configuration) {
8080
pca.configuration = pc
81-
r, _ := regexp.Compile(pca.configuration.NeedsRetitle.Regexp)
82-
pca.plugin.SetConfig(pc.NeedsRetitle.ErrorMessage, r)
81+
82+
if len(pc.NeedsRetitle.Regexp) > 0 {
83+
r, _ := regexp.Compile(pca.configuration.NeedsRetitle.Regexp)
84+
pca.plugin.SetConfig(pc.NeedsRetitle.ErrorMessage, r)
85+
}
8386
}
8487

8588
func (c *Configuration) Validate() error {
8689
if len(c.NeedsRetitle.Regexp) == 0 {
87-
return fmt.Errorf("needs_pr_rename.regexp can not be empty")
90+
logrus.Warning("empty regular expression provided, the plugin won't do anything")
91+
return nil
8892
}
8993

9094
_, err := regexp.Compile(c.NeedsRetitle.Regexp)

‎pkg/config/config_test.go

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
package config
2+
3+
import (
4+
"testing"
5+
6+
"github.com/stretchr/testify/assert"
7+
)
8+
9+
func TestConfigValidate(t *testing.T) {
10+
pca := NewPluginConfigAgent()
11+
12+
err := pca.Load("test/noconfig.yaml")
13+
14+
assert.NoError(t, err)
15+
16+
assert.Nil(t, pca.plugin.GetConfig())
17+
18+
err = pca.Load("test/wrongconfig.yaml")
19+
20+
assert.Error(t, err)
21+
22+
assert.Nil(t, pca.plugin.GetConfig())
23+
24+
err = pca.Load("test/config.yaml")
25+
26+
assert.NoError(t, err)
27+
28+
assert.NotNil(t, pca.plugin.GetConfig())
29+
}

‎pkg/config/test/config.yaml

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
needs_retitle:
2+
regexp: "^(fix:|feat:|major:).*$"
3+
error_message: "blah blah"

‎pkg/config/test/noconfig.yaml

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
plugins:
2+
bleh/bleh:
3+
- blah
4+
- blih

‎pkg/config/test/wrongconfig.yaml

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
needs_retitle:
2+
regexp: "(?'bkeh)"
3+
error_message: "blah blah"

‎pkg/plugin/plugin.go

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,7 @@ func (p *Plugin) SetConfig(m string, r *regexp.Regexp) {
8989
}
9090
}
9191

92-
func (p *Plugin) getConfig() *pluginConfig {
92+
func (p *Plugin) GetConfig() *pluginConfig {
9393
p.mut.Lock()
9494
defer p.mut.Unlock()
9595
return p.c
@@ -129,6 +129,13 @@ func (p *Plugin) handle(log *logrus.Entry, ghc githubClient, pr *github.PullRequ
129129
return nil
130130
}
131131

132+
c := p.GetConfig()
133+
134+
if c == nil {
135+
log.Warnf("No regular expression provided, please check your settings")
136+
return nil
137+
}
138+
132139
org := pr.Base.Repo.Owner.Login
133140
repo := pr.Base.Repo.Name
134141
number := pr.Number
@@ -140,13 +147,21 @@ func (p *Plugin) handle(log *logrus.Entry, ghc githubClient, pr *github.PullRequ
140147
}
141148
hasLabel := github.HasLabel(needsRetitleLabel, issueLabels)
142149

143-
return p.takeAction(log, ghc, org, repo, number, pr.User.Login, hasLabel, title)
150+
return p.takeAction(log, ghc, org, repo, number, pr.User.Login, hasLabel, title, c)
144151
}
145152

146153
// HandleAll checks all orgs and repos that enabled this plugin for open PRs to
147154
// determine if the "needs-retitle" label needs to be added or removed.
148155
func (p *Plugin) HandleAll(log *logrus.Entry, ghc githubClient, config *plugins.Configuration) error {
149156
log.Info("Checking all PRs.")
157+
158+
c := p.GetConfig()
159+
160+
if c == nil {
161+
log.Warnf("No regular expression provided, please check your settings")
162+
return nil
163+
}
164+
150165
orgs, repos := config.EnabledReposForExternalPlugin(PluginName)
151166
if len(orgs) == 0 && len(repos) == 0 {
152167
log.Warnf("No repos have been configured for the %s plugin", PluginName)
@@ -192,6 +207,7 @@ func (p *Plugin) HandleAll(log *logrus.Entry, ghc githubClient, config *plugins.
192207
string(pr.Author.Login),
193208
hasLabel,
194209
title,
210+
c,
195211
)
196212
if err != nil {
197213
l.WithError(err).Error("Error handling PR.")
@@ -203,8 +219,7 @@ func (p *Plugin) HandleAll(log *logrus.Entry, ghc githubClient, config *plugins.
203219
// takeAction adds or removes the "needs-rebase" label based on the current
204220
// state of the PR (hasLabel and mergeable). It also handles adding and
205221
// removing GitHub comments notifying the PR author that a rebase is needed.
206-
func (p *Plugin) takeAction(log *logrus.Entry, ghc githubClient, org, repo string, num int, author string, hasLabel bool, title string) error {
207-
c := p.getConfig()
222+
func (p *Plugin) takeAction(log *logrus.Entry, ghc githubClient, org, repo string, num int, author string, hasLabel bool, title string, c *pluginConfig) error {
208223
needsRetitleMessage := c.errorMessage
209224
titleOk := c.re.MatchString(title)
210225
if !titleOk && !hasLabel {

‎pkg/plugin/plugin_test.go

Lines changed: 39 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -142,7 +142,6 @@ func (f *fghc) compareExpected(t *testing.T, org, repo string, num int, expected
142142
}
143143

144144
func TestHandleIssueCommentEvent(t *testing.T) {
145-
146145
pr := func(s string) *github.PullRequest {
147146
pr := github.PullRequest{
148147
Base: github.PullRequestBranch{
@@ -161,20 +160,9 @@ func TestHandleIssueCommentEvent(t *testing.T) {
161160
sleep = func(time.Duration) { return }
162161
defer func() { sleep = oldSleep }()
163162

164-
r, err := regexp.Compile("^(fix:|feat:|major:).*$")
165-
if err != nil {
166-
t.Fatalf("error while compiling regular expression: %v", err)
167-
}
168-
169-
testSubject := &Plugin{
170-
c: &pluginConfig{
171-
errorMessage: fmt.Sprintf(defaultNeedsRetitleMessage, r.String()),
172-
re: r,
173-
},
174-
}
175-
176163
testCases := []struct {
177164
name string
165+
re string
178166
pr *github.PullRequest
179167

180168
merged bool
@@ -187,27 +175,37 @@ func TestHandleIssueCommentEvent(t *testing.T) {
187175
}{
188176
{
189177
name: "No pull request, ignoring",
178+
re: "^(fix:|feat:|major:).*$",
190179
},
191180
{
192181
name: "correct title no-op",
182+
re: "^(fix:|feat:|major:).*$",
193183
pr: pr("fix: this is a valid title"),
194184
labels: []string{labels.LGTM, labels.NeedsSig},
195185
},
196186
{
197187
name: "wrong title no-op",
188+
re: "^(fix:|feat:|major:).*$",
198189
pr: pr("this title is wrong..."),
199190
labels: []string{labels.LGTM, labels.NeedsSig, needsRetitleLabel},
200191
},
201192
{
202193
name: "wrong title adds label",
194+
re: "^(fix:|feat:|major:).*$",
203195
pr: pr("this title is wrong..."),
204196
labels: []string{labels.LGTM, labels.NeedsSig},
205197

206198
expectedAdded: []string{needsRetitleLabel},
207199
expectComment: true,
208200
},
201+
{
202+
name: "wrong title no config ignores it",
203+
pr: pr("this title is wrong..."),
204+
labels: []string{labels.LGTM, labels.NeedsSig},
205+
},
209206
{
210207
name: "valid title removes label",
208+
re: "^(fix:|feat:|major:).*$",
211209
pr: pr("feat: this is valid title"),
212210
labels: []string{labels.LGTM, labels.NeedsSig, needsRetitleLabel},
213211

@@ -216,13 +214,22 @@ func TestHandleIssueCommentEvent(t *testing.T) {
216214
},
217215
{
218216
name: "merged pr is ignored",
217+
re: "^(fix:|feat:|major:).*$",
219218
pr: pr("bleh"),
220219
merged: true,
221220
},
222221
}
223222

224223
for _, tc := range testCases {
225224
t.Run(tc.name, func(t *testing.T) {
225+
testSubject := &Plugin{}
226+
if len(tc.re) > 0 {
227+
r, _ := regexp.Compile(tc.re)
228+
testSubject.c = &pluginConfig{
229+
errorMessage: fmt.Sprintf(defaultNeedsRetitleMessage, "some regexp"),
230+
re: r,
231+
}
232+
}
226233
fake := newFakeClient(nil, tc.labels, tc.pr)
227234
ice := &github.IssueCommentEvent{}
228235
if tc.pr != nil {
@@ -242,20 +249,9 @@ func TestHandlePullRequestEvent(t *testing.T) {
242249
sleep = func(time.Duration) { return }
243250
defer func() { sleep = oldSleep }()
244251

245-
r, err := regexp.Compile("^(fix:|feat:|major:).*$")
246-
if err != nil {
247-
t.Fatalf("error while compiling regular expression: %v", err)
248-
}
249-
250-
testSubject := &Plugin{
251-
c: &pluginConfig{
252-
errorMessage: fmt.Sprintf(defaultNeedsRetitleMessage, r.String()),
253-
re: r,
254-
},
255-
}
256-
257252
testCases := []struct {
258253
name string
254+
re string
259255

260256
title string
261257
merged bool
@@ -268,24 +264,33 @@ func TestHandlePullRequestEvent(t *testing.T) {
268264
}{
269265
{
270266
name: "correct title no-op",
267+
re: "^(fix:|feat:|major:).*$",
271268
title: "fix: this is a valid title",
272269
labels: []string{labels.LGTM, labels.NeedsSig},
273270
},
274271
{
275272
name: "wrong title no-op",
273+
re: "^(fix:|feat:|major:).*$",
276274
title: "fixing: wrong title",
277275
labels: []string{labels.LGTM, labels.NeedsSig, needsRetitleLabel},
278276
},
279277
{
280278
name: "wrong title adds label",
279+
re: "^(fix:|feat:|major:).*$",
281280
title: "fixing: wrong title",
282281
labels: []string{labels.LGTM, labels.NeedsSig},
283282

284283
expectedAdded: []string{needsRetitleLabel},
285284
expectComment: true,
286285
},
286+
{
287+
name: "wrong title, no config ignores it",
288+
title: "fixing: wrong title",
289+
labels: []string{labels.LGTM, labels.NeedsSig},
290+
},
287291
{
288292
name: "correct title removes label",
293+
re: "^(fix:|feat:|major:).*$",
289294
title: "major: this is a valid title",
290295
labels: []string{labels.LGTM, labels.NeedsSig, needsRetitleLabel},
291296

@@ -294,11 +299,20 @@ func TestHandlePullRequestEvent(t *testing.T) {
294299
},
295300
{
296301
name: "merged pr is ignored",
302+
re: "^(fix:|feat:|major:).*$",
297303
merged: true,
298304
},
299305
}
300306

301307
for _, tc := range testCases {
308+
testSubject := &Plugin{}
309+
if len(tc.re) > 0 {
310+
r, _ := regexp.Compile(tc.re)
311+
testSubject.c = &pluginConfig{
312+
errorMessage: fmt.Sprintf(defaultNeedsRetitleMessage, "some regexp"),
313+
re: r,
314+
}
315+
}
302316
fake := newFakeClient(nil, tc.labels, nil)
303317
pre := &github.PullRequestEvent{
304318
Action: github.PullRequestActionSynchronize,

0 commit comments

Comments
 (0)