Skip to content

Commit 238d2cf

Browse files
chmouelchmouel
committed
fix: Ignore non-Tekton resources errors reporting
unmarshal non-Tekton custom resources, such as `ImageDigestMirrorSet`, or `StepActions` as PipelineRuns. This caused the PaC bot to post misleading "PipelineRun failure" comments on pull requests, even when all CI jobs were successful. This change updates the parsing logic to specifically check for and ignore non-Tekton resources found within the `.tekton` directory. By skipping these files, we prevent erroneous parsing errors and stop the bot from posting false-positive comments. This resolves the issue where developers were confused by failure notifications on otherwise successful PRs, improving the overall developer experience by reducing unnecessary noise. The fix ensures that comments are only posted for legitimate PipelineRun failures. Jira: https://issues.redhat.com/browse/SRVKP-8112 Signed-off-by: Chmouel Boudjnah <[email protected]>
1 parent 25d6e84 commit 238d2cf

File tree

3 files changed

+286
-53
lines changed

3 files changed

+286
-53
lines changed

‎pkg/pipelineascode/errors.go‎

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
package pipelineascode
2+
3+
import (
4+
"context"
5+
"fmt"
6+
"regexp"
7+
"strings"
8+
9+
"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1"
10+
pacerrors "github.com/openshift-pipelines/pipelines-as-code/pkg/errors"
11+
"github.com/openshift-pipelines/pipelines-as-code/pkg/provider"
12+
tektonv1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
13+
"go.uber.org/zap"
14+
)
15+
16+
const validationErrorTemplate = `> [!CAUTION]
17+
> There are some errors in your PipelineRun template.
18+
19+
| PipelineRun | Error |
20+
|------|-------|`
21+
22+
var regexpIgnoreErrors = regexp.MustCompile(`.*no kind.*is registered for version.*in scheme.*`)
23+
24+
func (p *PacRun) checkAccessOrErrror(ctx context.Context, repo *v1alpha1.Repository, status provider.StatusOpts, viamsg string) (bool, error) {
25+
allowed, err := p.vcx.IsAllowed(ctx, p.event)
26+
if err != nil {
27+
return false, fmt.Errorf("unable to verify event authorization: %w", err)
28+
}
29+
if allowed {
30+
return true, nil
31+
}
32+
msg := fmt.Sprintf("User %s is not allowed to trigger CI %s in this repo.", p.event.Sender, viamsg)
33+
if p.event.AccountID != "" {
34+
msg = fmt.Sprintf("User: %s AccountID: %s is not allowed to trigger CI %s in this repo.", p.event.Sender, p.event.AccountID, viamsg)
35+
}
36+
p.eventEmitter.EmitMessage(repo, zap.InfoLevel, "RepositoryPermissionDenied", msg)
37+
status.Text = msg
38+
39+
if err := p.vcx.CreateStatus(ctx, p.event, status); err != nil {
40+
return false, fmt.Errorf("failed to run create status, user is not allowed to run the CI:: %w", err)
41+
}
42+
return false, nil
43+
}
44+
45+
// reportValidationErrors reports validation errors found in PipelineRuns by:
46+
// 1. Creating error messages for each validation error
47+
// 2. Emitting error messages to the event system
48+
// 3. Creating a markdown formatted comment on the repository with all errors.
49+
func (p *PacRun) reportValidationErrors(ctx context.Context, repo *v1alpha1.Repository, validationErrors []*pacerrors.PacYamlValidations) {
50+
errorRows := make([]string, 0, len(validationErrors))
51+
for _, err := range validationErrors {
52+
// if the error is a TektonConversionError, we don't want to report it since it may be a file that is not a tekton resource
53+
// and we don't want to report it as a validation error.
54+
if !regexpIgnoreErrors.MatchString(err.Err.Error()) && (strings.HasPrefix(err.Schema, tektonv1.SchemeGroupVersion.Group) || err.Schema == pacerrors.GenericBadYAMLValidation) {
55+
errorRows = append(errorRows, fmt.Sprintf("| %s | `%s` |", err.Name, err.Err.Error()))
56+
}
57+
p.eventEmitter.EmitMessage(repo, zap.ErrorLevel, "PipelineRunValidationErrors",
58+
fmt.Sprintf("cannot read the PipelineRun: %s, error: %s", err.Name, err.Err.Error()))
59+
}
60+
if len(errorRows) == 0 {
61+
return
62+
}
63+
markdownErrMessage := fmt.Sprintf(`%s
64+
%s`, validationErrorTemplate, strings.Join(errorRows, "\n"))
65+
if err := p.vcx.CreateComment(ctx, p.event, markdownErrMessage, validationErrorTemplate); err != nil {
66+
p.eventEmitter.EmitMessage(repo, zap.ErrorLevel, "PipelineRunCommentCreationError",
67+
fmt.Sprintf("failed to create comment: %s", err.Error()))
68+
}
69+
}

‎pkg/pipelineascode/errors_test.go‎

Lines changed: 217 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,217 @@
1+
package pipelineascode
2+
3+
import (
4+
"context"
5+
"errors"
6+
"testing"
7+
8+
"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1"
9+
pacerrors "github.com/openshift-pipelines/pipelines-as-code/pkg/errors"
10+
"github.com/openshift-pipelines/pipelines-as-code/pkg/events"
11+
"github.com/openshift-pipelines/pipelines-as-code/pkg/params/info"
12+
"github.com/openshift-pipelines/pipelines-as-code/pkg/provider"
13+
testclient "github.com/openshift-pipelines/pipelines-as-code/pkg/test/clients"
14+
testprovider "github.com/openshift-pipelines/pipelines-as-code/pkg/test/provider"
15+
"go.uber.org/zap"
16+
zapobserver "go.uber.org/zap/zaptest/observer"
17+
"gotest.tools/v3/assert"
18+
rtesting "knative.dev/pkg/reconciler/testing"
19+
)
20+
21+
func TestCheckAccessOrErrror(t *testing.T) {
22+
tests := []struct {
23+
name string
24+
allowIt bool
25+
sender string
26+
accountID string
27+
createStatusError bool
28+
expectedErr bool
29+
expectedAllowed bool
30+
expectedErrMsg string
31+
}{
32+
{
33+
name: "user is allowed",
34+
allowIt: true,
35+
expectedAllowed: true,
36+
},
37+
{
38+
name: "user is not allowed - no account ID",
39+
allowIt: false,
40+
sender: "johndoe",
41+
expectedAllowed: false,
42+
},
43+
{
44+
name: "user is not allowed - with account ID",
45+
allowIt: false,
46+
sender: "johndoe",
47+
accountID: "user123",
48+
expectedAllowed: false,
49+
},
50+
{
51+
name: "create status error",
52+
allowIt: false,
53+
sender: "johndoe",
54+
createStatusError: true,
55+
expectedErr: true,
56+
expectedErrMsg: "failed to run create status",
57+
},
58+
}
59+
60+
for _, tt := range tests {
61+
t.Run(tt.name, func(t *testing.T) {
62+
// Setup observer to capture logs
63+
observerCore, _ := zapobserver.New(zap.InfoLevel)
64+
logger := zap.New(observerCore).Sugar()
65+
66+
// Create test event
67+
testEvent := &info.Event{
68+
Sender: tt.sender,
69+
AccountID: tt.accountID,
70+
}
71+
72+
// Create mock provider
73+
prov := &testprovider.TestProviderImp{
74+
AllowIT: tt.allowIt,
75+
}
76+
77+
// Set createStatus error if needed
78+
if tt.createStatusError {
79+
prov.CreateStatusErorring = true
80+
}
81+
82+
ctx, _ := rtesting.SetupFakeContext(t)
83+
stdata, _ := testclient.SeedTestData(t, ctx, testclient.Data{})
84+
// Create mock event emitter
85+
eventEmitter := events.NewEventEmitter(stdata.Kube, logger)
86+
87+
// Create PacRun
88+
p := &PacRun{
89+
event: testEvent,
90+
vcx: prov,
91+
logger: logger,
92+
eventEmitter: eventEmitter,
93+
}
94+
95+
// Call the function
96+
repo := &v1alpha1.Repository{}
97+
status := provider.StatusOpts{}
98+
allowed, err := p.checkAccessOrErrror(context.Background(), repo, status, "via test")
99+
100+
// Verify results
101+
if tt.expectedErr {
102+
assert.Assert(t, err != nil, "Expected error but got nil")
103+
if tt.expectedErrMsg != "" {
104+
assert.Assert(t, err.Error() != "", "Expected error message but got empty string")
105+
assert.ErrorContains(t, err, tt.expectedErrMsg)
106+
}
107+
} else {
108+
assert.NilError(t, err)
109+
}
110+
111+
assert.Equal(t, tt.expectedAllowed, allowed)
112+
})
113+
}
114+
}
115+
116+
func TestReportValidationErrors(t *testing.T) {
117+
tests := []struct {
118+
name string
119+
validationErrors []*pacerrors.PacYamlValidations
120+
expectCommentCreation bool
121+
}{
122+
{
123+
name: "no validation errors",
124+
validationErrors: []*pacerrors.PacYamlValidations{},
125+
expectCommentCreation: false,
126+
},
127+
{
128+
name: "tekton validation errors",
129+
validationErrors: []*pacerrors.PacYamlValidations{
130+
{
131+
Name: "test-pipeline-1",
132+
Err: errors.New("invalid pipeline spec"),
133+
Schema: "tekton.dev",
134+
},
135+
},
136+
expectCommentCreation: true,
137+
},
138+
{
139+
name: "non-tekton schema errors",
140+
validationErrors: []*pacerrors.PacYamlValidations{
141+
{
142+
Name: "test-other",
143+
Err: errors.New("some error"),
144+
Schema: "other.schema",
145+
},
146+
},
147+
expectCommentCreation: false,
148+
},
149+
{
150+
name: "ignored errors by regex",
151+
validationErrors: []*pacerrors.PacYamlValidations{
152+
{
153+
Name: "test-ignored",
154+
Err: errors.New("no kind test is registered for version v1 in scheme"),
155+
Schema: "tekton.dev",
156+
},
157+
},
158+
expectCommentCreation: false,
159+
},
160+
{
161+
name: "create comment error",
162+
validationErrors: []*pacerrors.PacYamlValidations{
163+
{
164+
Name: "test-pipeline",
165+
Err: errors.New("validation error"),
166+
Schema: "tekton.dev",
167+
},
168+
},
169+
expectCommentCreation: true,
170+
},
171+
}
172+
173+
for _, tt := range tests {
174+
t.Run(tt.name, func(t *testing.T) {
175+
// Setup observer to capture logs
176+
observerCore, logs := zapobserver.New(zap.InfoLevel)
177+
logger := zap.New(observerCore).Sugar()
178+
ctx, _ := rtesting.SetupFakeContext(t)
179+
180+
// Create test event
181+
testEvent := &info.Event{}
182+
183+
// Create mock provider
184+
prov := &testprovider.TestProviderImp{}
185+
186+
stdata, _ := testclient.SeedTestData(t, ctx, testclient.Data{})
187+
188+
// Create mock event emitter
189+
eventEmitter := events.NewEventEmitter(stdata.Kube, logger)
190+
// Create PacRun
191+
p := &PacRun{
192+
event: testEvent,
193+
vcx: prov,
194+
logger: logger,
195+
eventEmitter: eventEmitter,
196+
}
197+
198+
// Call the function
199+
repo := &v1alpha1.Repository{}
200+
p.reportValidationErrors(context.Background(), repo, tt.validationErrors)
201+
202+
// Verify results
203+
// Verify log messages for validation errors
204+
logEntries := logs.All()
205+
errorLogCount := 0
206+
for _, entry := range logEntries {
207+
if entry.Level == zap.ErrorLevel {
208+
errorLogCount++
209+
}
210+
}
211+
212+
// We should have at least one error log per validation error
213+
assert.Assert(t, errorLogCount >= len(tt.validationErrors),
214+
"Expected at least %d error logs, got %d", len(tt.validationErrors), errorLogCount)
215+
})
216+
}
217+
}

‎pkg/pipelineascode/match.go‎

Lines changed: 0 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -22,12 +22,6 @@ import (
2222
"go.uber.org/zap"
2323
)
2424

25-
const validationErrorTemplate = `> [!CAUTION]
26-
> There are some errors in your PipelineRun template.
27-
28-
| PipelineRun | Error |
29-
|------|-------|`
30-
3125
func (p *PacRun) matchRepoPR(ctx context.Context) ([]matcher.Match, *v1alpha1.Repository, error) {
3226
repo, err := p.verifyRepoAndUser(ctx)
3327
if err != nil {
@@ -432,27 +426,6 @@ func (p *PacRun) checkNeedUpdate(_ string) (string, bool) {
432426
return "", false
433427
}
434428

435-
func (p *PacRun) checkAccessOrErrror(ctx context.Context, repo *v1alpha1.Repository, status provider.StatusOpts, viamsg string) (bool, error) {
436-
allowed, err := p.vcx.IsAllowed(ctx, p.event)
437-
if err != nil {
438-
return false, fmt.Errorf("unable to verify event authorization: %w", err)
439-
}
440-
if allowed {
441-
return true, nil
442-
}
443-
msg := fmt.Sprintf("User %s is not allowed to trigger CI %s in this repo.", p.event.Sender, viamsg)
444-
if p.event.AccountID != "" {
445-
msg = fmt.Sprintf("User: %s AccountID: %s is not allowed to trigger CI %s in this repo.", p.event.Sender, p.event.AccountID, viamsg)
446-
}
447-
p.eventEmitter.EmitMessage(repo, zap.InfoLevel, "RepositoryPermissionDenied", msg)
448-
status.Text = msg
449-
450-
if err := p.vcx.CreateStatus(ctx, p.event, status); err != nil {
451-
return false, fmt.Errorf("failed to run create status, user is not allowed to run the CI:: %w", err)
452-
}
453-
return false, nil
454-
}
455-
456429
func (p *PacRun) createNeutralStatus(ctx context.Context) error {
457430
status := provider.StatusOpts{
458431
Status: CompletedStatus,
@@ -467,29 +440,3 @@ func (p *PacRun) createNeutralStatus(ctx context.Context) error {
467440

468441
return nil
469442
}
470-
471-
// reportValidationErrors reports validation errors found in PipelineRuns by:
472-
// 1. Creating error messages for each validation error
473-
// 2. Emitting error messages to the event system
474-
// 3. Creating a markdown formatted comment on the repository with all errors.
475-
func (p *PacRun) reportValidationErrors(ctx context.Context, repo *v1alpha1.Repository, validationErrors []*pacerrors.PacYamlValidations) {
476-
errorRows := make([]string, 0, len(validationErrors))
477-
for _, err := range validationErrors {
478-
// if the error is a TektonConversionError, we don't want to report it since it may be a file that is not a tekton resource
479-
// and we don't want to report it as a validation error.
480-
if strings.HasPrefix(err.Schema, tektonv1.SchemeGroupVersion.Group) || err.Schema == pacerrors.GenericBadYAMLValidation {
481-
errorRows = append(errorRows, fmt.Sprintf("| %s | `%s` |", err.Name, err.Err.Error()))
482-
}
483-
p.eventEmitter.EmitMessage(repo, zap.ErrorLevel, "PipelineRunValidationErrors",
484-
fmt.Sprintf("cannot read the PipelineRun: %s, error: %s", err.Name, err.Err.Error()))
485-
}
486-
if len(errorRows) == 0 {
487-
return
488-
}
489-
markdownErrMessage := fmt.Sprintf(`%s
490-
%s`, validationErrorTemplate, strings.Join(errorRows, "\n"))
491-
if err := p.vcx.CreateComment(ctx, p.event, markdownErrMessage, validationErrorTemplate); err != nil {
492-
p.eventEmitter.EmitMessage(repo, zap.ErrorLevel, "PipelineRunCommentCreationError",
493-
fmt.Sprintf("failed to create comment: %s", err.Error()))
494-
}
495-
}

0 commit comments

Comments
 (0)