Skip to content

Commit 66e5059

Browse files
chmouelchmouel
committed
fix handle multiline trigger_comment var properly
When using `{{ trigger_comment }}` in a PipelineRun YAML, GitHub comments containing newlines (e.g., `/help`) can break YAML parsing due to improper expansion. This change replaces `\r\n` with `\n` in `trigger_comment`, similar to how `pull_requests_labels` are handled. This ensures valid YAML formatting and avoids parser errors. Fixes #1929 Signed-off-by: Chmouel Boudjnah <[email protected]>
1 parent cdd88ce commit 66e5059

7 files changed

+192
-77
lines changed

‎docs/content/docs/guide/gitops_commands.md

+31-7
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ Roses are red, violets are blue. Pipelines are bound to flake by design.
3131

3232
{{< hint info >}}
3333

34-
Please be aware that GitOps commands such as `/test` and others will not function on closed Pull Requests or Merge Requests.
34+
Please be aware that GitOps commands such as `/test` and others will not function on closed Pull Requests or Merge Requests.
3535

3636
{{< /hint >}}
3737

@@ -87,21 +87,45 @@ The PipelineRun will be restarted regardless of the annotations if the comment `
8787

8888
## Accessing the Comment Triggering the PipelineRun
8989

90-
When you trigger a PipelineRun via a GitOps Command, the template variable `{{ trigger_comment }}` is set to the actual comment that triggered it.
90+
When you trigger a PipelineRun via a GitOps Command, the template variable `{{
91+
trigger_comment }}` is set to the actual comment that triggered it.
9192

92-
You can then perform actions based on the comment content with a shell script or other methods.
93+
You can then perform actions based on the comment content with a shell script
94+
or other methods.
9395

94-
There is a restriction with the `trigger_comment` variable: we modify it to replace newlines with `\n` since multi-line comments can cause issues when replaced inside the YAML.
96+
Expanding `{{ trigger_comment }}` in YAML can break parsing if the comment
97+
contains newlines. For example, a GitHub comment like:
9598

96-
It is up to you to replace it back with newlines. For example, with shell scripts, you can use `echo -e` to expand the newline back.
99+
```console
100+
/help
97101

98-
Example of a shell script:
102+
This is a test.
103+
```
104+
105+
Expands to:
106+
107+
```yaml
108+
params:
109+
- name: trigger_comment
110+
value: "/help
111+
112+
This is a test."
113+
```
114+
115+
The empty line makes the YAML invalid. To prevent this, we replace `\r\n` with
116+
`\n` to ensure proper formatting.
117+
118+
You can restore the newlines in your task as needed.
119+
120+
For example, in a shell script, use `echo -e` to expand `\n` back into actual newlines:
99121

100122
```shell
101123
echo -e "{{ trigger_comment }}" > /tmp/comment
102-
grep "string" /tmp/comment
124+
grep "/help" /tmp/comment # will output only /help
103125
```
104126

127+
This ensures the comment is correctly formatted when processed.
128+
105129
## Custom GitOps Commands
106130

107131
Using the [on-comment]({{< relref "/docs/guide/matchingevents.md#matching-a-pipelinerun-on-a-regex-in-a-comment" >}}) annotation on your `PipelineRun`, you can define custom GitOps commands that will be triggered by comments on the Pull Request.

‎pkg/customparams/standard.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ func (p *CustomParams) makeStandardParamsFromEvent(ctx context.Context) (map[str
3333
repoURL = p.event.CloneURL
3434
}
3535
changedFiles := p.getChangedFiles(ctx)
36-
triggerCommentAsSingleLine := strings.ReplaceAll(p.event.TriggerComment, "\n", "\\n")
36+
triggerCommentAsSingleLine := strings.ReplaceAll(strings.ReplaceAll(p.event.TriggerComment, "\r\n", "\\n"), "\n", "\\n")
3737
pullRequestLabels := strings.Join(p.event.PullRequestLabel, "\\n")
3838

3939
return map[string]string{

‎pkg/customparams/standard_test.go

+149-63
Original file line numberDiff line numberDiff line change
@@ -12,71 +12,157 @@ import (
1212
)
1313

1414
func TestMakeStandardParamsFromEvent(t *testing.T) {
15-
event := &info.Event{
16-
SHA: "1234567890",
17-
Organization: "Org",
18-
Repository: "Repo",
19-
BaseBranch: "main",
20-
HeadBranch: "foo",
21-
EventType: "pull_request",
22-
Sender: "SENDER",
23-
URL: "https://paris.com",
24-
HeadURL: "https://india.com",
25-
TriggerComment: "/test me\nHelp me obiwan kenobi",
26-
PullRequestLabel: []string{"bugs", "enhancements"},
27-
}
28-
29-
result := map[string]string{
30-
"event_type": "pull_request",
31-
"repo_name": "repo",
32-
"repo_owner": "org",
33-
"repo_url": "https://paris.com",
34-
"source_url": "https://india.com",
35-
"revision": "1234567890",
36-
"sender": "sender",
37-
"source_branch": "foo",
38-
"target_branch": "main",
39-
"target_namespace": "myns",
40-
"trigger_comment": "/test me\\nHelp me obiwan kenobi",
41-
"pull_request_labels": "bugs\\nenhancements",
42-
}
43-
44-
repo := &v1alpha1.Repository{
45-
ObjectMeta: metav1.ObjectMeta{
46-
Name: "myname",
47-
Namespace: "myns",
15+
tests := []struct {
16+
name string
17+
event *info.Event
18+
repo *v1alpha1.Repository
19+
want map[string]string
20+
wantVCX *testprovider.TestProviderImp
21+
}{
22+
{
23+
name: "basic event test",
24+
event: &info.Event{
25+
SHA: "1234567890",
26+
Organization: "Org",
27+
Repository: "Repo",
28+
BaseBranch: "main",
29+
HeadBranch: "foo",
30+
EventType: "pull_request",
31+
Sender: "SENDER",
32+
URL: "https://paris.com",
33+
HeadURL: "https://india.com",
34+
TriggerComment: "\n/test me\nHelp me obiwan kenobi\r\n\r\n\r\nTo test or not to test, is the question?\n\n\n",
35+
PullRequestLabel: []string{"bugs", "enhancements"},
36+
},
37+
repo: &v1alpha1.Repository{
38+
ObjectMeta: metav1.ObjectMeta{
39+
Name: "myname",
40+
Namespace: "myns",
41+
},
42+
},
43+
want: map[string]string{
44+
"event_type": "pull_request",
45+
"repo_name": "repo",
46+
"repo_owner": "org",
47+
"repo_url": "https://paris.com",
48+
"source_url": "https://india.com",
49+
"revision": "1234567890",
50+
"sender": "sender",
51+
"source_branch": "foo",
52+
"target_branch": "main",
53+
"target_namespace": "myns",
54+
"trigger_comment": `\n/test me\nHelp me obiwan kenobi\n\n\nTo test or not to test, is the question?\n\n\n`,
55+
"pull_request_labels": "bugs\\nenhancements",
56+
},
57+
wantVCX: &testprovider.TestProviderImp{
58+
WantAllChangedFiles: []string{"added.go", "deleted.go", "modified.go", "renamed.go"},
59+
WantAddedFiles: []string{"added.go"},
60+
WantDeletedFiles: []string{"deleted.go"},
61+
WantModifiedFiles: []string{"modified.go"},
62+
WantRenamedFiles: []string{"renamed.go"},
63+
},
64+
},
65+
{
66+
name: "basic event test",
67+
event: &info.Event{
68+
SHA: "1234567890",
69+
Organization: "Org",
70+
Repository: "Repo",
71+
BaseBranch: "main",
72+
HeadBranch: "foo",
73+
EventType: "pull_request",
74+
Sender: "SENDER",
75+
URL: "https://paris.com",
76+
HeadURL: "https://india.com",
77+
TriggerComment: "/test me\nHelp me obiwan kenobi",
78+
PullRequestLabel: []string{"bugs", "enhancements"},
79+
},
80+
repo: &v1alpha1.Repository{
81+
ObjectMeta: metav1.ObjectMeta{
82+
Name: "myname",
83+
Namespace: "myns",
84+
},
85+
},
86+
want: map[string]string{
87+
"event_type": "pull_request",
88+
"repo_name": "repo",
89+
"repo_owner": "org",
90+
"repo_url": "https://paris.com",
91+
"source_url": "https://india.com",
92+
"revision": "1234567890",
93+
"sender": "sender",
94+
"source_branch": "foo",
95+
"target_branch": "main",
96+
"target_namespace": "myns",
97+
"trigger_comment": "/test me\\nHelp me obiwan kenobi",
98+
"pull_request_labels": "bugs\\nenhancements",
99+
},
100+
wantVCX: &testprovider.TestProviderImp{
101+
WantAllChangedFiles: []string{"added.go", "deleted.go", "modified.go", "renamed.go"},
102+
WantAddedFiles: []string{"added.go"},
103+
WantDeletedFiles: []string{"deleted.go"},
104+
WantModifiedFiles: []string{"modified.go"},
105+
WantRenamedFiles: []string{"renamed.go"},
106+
},
107+
},
108+
{
109+
name: "event with different clone URL",
110+
event: &info.Event{
111+
SHA: "1234567890",
112+
Organization: "Org",
113+
Repository: "Repo",
114+
BaseBranch: "main",
115+
HeadBranch: "foo",
116+
EventType: "pull_request",
117+
Sender: "SENDER",
118+
URL: "https://paris.com",
119+
HeadURL: "https://india.com",
120+
TriggerComment: "/test me\nHelp me obiwan kenobi",
121+
PullRequestLabel: []string{"bugs", "enhancements"},
122+
CloneURL: "https://blahblah",
123+
},
124+
repo: &v1alpha1.Repository{
125+
ObjectMeta: metav1.ObjectMeta{
126+
Name: "myname",
127+
Namespace: "myns",
128+
},
129+
},
130+
want: map[string]string{
131+
"event_type": "pull_request",
132+
"repo_name": "repo",
133+
"repo_owner": "org",
134+
"repo_url": "https://blahblah",
135+
"source_url": "https://india.com",
136+
"revision": "1234567890",
137+
"sender": "sender",
138+
"source_branch": "foo",
139+
"target_branch": "main",
140+
"target_namespace": "myns",
141+
"trigger_comment": "/test me\\nHelp me obiwan kenobi",
142+
"pull_request_labels": "bugs\\nenhancements",
143+
},
144+
wantVCX: &testprovider.TestProviderImp{
145+
WantAllChangedFiles: []string{"added.go", "deleted.go", "modified.go", "renamed.go"},
146+
WantAddedFiles: []string{"added.go"},
147+
WantDeletedFiles: []string{"deleted.go"},
148+
WantModifiedFiles: []string{"modified.go"},
149+
WantRenamedFiles: []string{"renamed.go"},
150+
},
48151
},
49152
}
50153

51-
ctx, _ := rectesting.SetupFakeContext(t)
52-
vcx := &testprovider.TestProviderImp{
53-
WantAllChangedFiles: []string{"added.go", "deleted.go", "modified.go", "renamed.go"},
54-
WantAddedFiles: []string{"added.go"},
55-
WantDeletedFiles: []string{"deleted.go"},
56-
WantModifiedFiles: []string{"modified.go"},
57-
WantRenamedFiles: []string{"renamed.go"},
58-
}
59-
60-
p := NewCustomParams(event, repo, nil, nil, nil, vcx)
61-
params, changedFiles := p.makeStandardParamsFromEvent(ctx)
62-
assert.DeepEqual(t, params, result)
63-
assert.DeepEqual(t, changedFiles["all"], vcx.WantAllChangedFiles)
64-
assert.DeepEqual(t, changedFiles["added"], vcx.WantAddedFiles)
65-
assert.DeepEqual(t, changedFiles["deleted"], vcx.WantDeletedFiles)
66-
assert.DeepEqual(t, changedFiles["modified"], vcx.WantModifiedFiles)
67-
assert.DeepEqual(t, changedFiles["renamed"], vcx.WantRenamedFiles)
154+
for _, tt := range tests {
155+
t.Run(tt.name, func(t *testing.T) {
156+
ctx, _ := rectesting.SetupFakeContext(t)
157+
p := NewCustomParams(tt.event, tt.repo, nil, nil, nil, tt.wantVCX)
158+
params, changedFiles := p.makeStandardParamsFromEvent(ctx)
68159

69-
nevent := &info.Event{}
70-
event.DeepCopyInto(nevent)
71-
nevent.CloneURL = "https://blahblah"
72-
p.event = nevent
73-
nparams, nchangedFiles := p.makeStandardParamsFromEvent(ctx)
74-
result["repo_url"] = nevent.CloneURL
75-
assert.DeepEqual(t, nparams, result)
76-
77-
assert.DeepEqual(t, nchangedFiles["all"], vcx.WantAllChangedFiles)
78-
assert.DeepEqual(t, nchangedFiles["added"], vcx.WantAddedFiles)
79-
assert.DeepEqual(t, nchangedFiles["deleted"], vcx.WantDeletedFiles)
80-
assert.DeepEqual(t, nchangedFiles["modified"], vcx.WantModifiedFiles)
81-
assert.DeepEqual(t, nchangedFiles["renamed"], vcx.WantRenamedFiles)
160+
assert.DeepEqual(t, params, tt.want)
161+
assert.DeepEqual(t, changedFiles["all"], tt.wantVCX.WantAllChangedFiles)
162+
assert.DeepEqual(t, changedFiles["added"], tt.wantVCX.WantAddedFiles)
163+
assert.DeepEqual(t, changedFiles["deleted"], tt.wantVCX.WantDeletedFiles)
164+
assert.DeepEqual(t, changedFiles["modified"], tt.wantVCX.WantModifiedFiles)
165+
assert.DeepEqual(t, changedFiles["renamed"], tt.wantVCX.WantRenamedFiles)
166+
})
167+
}
82168
}

‎test/gitea_gitops_commands_test.go

+4-3
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ func TestGiteaOnCommentAnnotation(t *testing.T) {
7070
},
7171
},
7272
}
73-
triggerComment := "/hello-world"
73+
triggerComment := "/hello-world\r\n\r\n"
7474
topts.TargetNS = topts.TargetRefName
7575
topts.ParamsRun, topts.Opts, topts.GiteaCNX, err = tgitea.Setup(ctx)
7676
assert.NilError(t, err, fmt.Errorf("cannot do gitea setup: %w", err))
@@ -108,8 +108,9 @@ func TestGiteaOnCommentAnnotation(t *testing.T) {
108108
assert.Equal(t, *repo.Status[len(repo.Status)-1].EventType, opscomments.OnCommentEventType.String(), "should have a on comment event type")
109109

110110
last := repo.Status[len(repo.Status)-1]
111-
err = twait.RegexpMatchingInPodLog(context.Background(), topts.ParamsRun, topts.TargetNS, fmt.Sprintf("tekton.dev/pipelineRun=%s", last.PipelineRunName), "step-task", *regexp.MustCompile(triggerComment), "", 2)
112-
assert.NilError(t, err)
111+
twait.GoldenPodLog(context.Background(), t, topts.ParamsRun, topts.TargetNS,
112+
fmt.Sprintf("tekton.dev/pipelineRun=%s", last.PipelineRunName),
113+
"step-task", strings.ReplaceAll(fmt.Sprintf("%s-pipelinerun-on-comment-annotation.golden", t.Name()), "/", "-"), 2)
113114

114115
tgitea.PostCommentOnPullRequest(t, topts, fmt.Sprintf(`%s revision=main custom1=thisone custom2="another one" custom3="a \"quote\""`, triggerComment))
115116
waitOpts.MinNumberStatus = 4

‎test/testdata/TestGiteaOnCommentAnnotation-pipelinerun-on-comment-annotation.golden

+6
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
The comment is:
2+
/hello-world\n\n
3+
The event is on-comment
4+
The custom1 value is foo
5+
The custom2 value is bar
6+
The custom3 value is moto

‎test/testdata/TestGiteaOnCommentAnnotation.golden

+1-2
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
The comment is:
2-
/hello-world revision=main custom1=thisone custom2="another one" custom3="a \"quote\""
2+
/hello-world\n\n revision=main custom1=thisone custom2="another one" custom3="a \"quote\""
33
The event is on-comment
4-
The revision is main
54
The custom1 value is thisone
65
The custom2 value is another one
76
The custom3 value is a quote

‎test/testdata/pipelinerun-on-comment-annotation.yaml

-1
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@ spec:
2121
{{ trigger_comment }}
2222
EOF
2323
echo "The event is {{ event_type }}"
24-
echo "The revision is {{ revision }}"
2524
echo "The custom1 value is {{ custom1 }}"
2625
echo "The custom2 value is {{ custom2 }}"
2726
echo "The custom3 value is {{ custom3 }}"

0 commit comments

Comments
 (0)