Skip to content

Commit 8072684

Browse files
Copilotfiftin
Copilot
and
fiftin
authored
Fix integration extracted values priority and ensure they are added when task environment is empty (#3253)
* Initial plan * Fix integration extracted values not being added when environment is empty Co-authored-by: fiftin <[email protected]> * Prioritize task definition variables over extracted values Co-authored-by: fiftin <[email protected]> * Addressing PR comments Co-authored-by: fiftin <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: fiftin <[email protected]>
1 parent b09ab2c commit 8072684

File tree

3 files changed

+266
-2
lines changed

3 files changed

+266
-2
lines changed

‎api/integration.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -289,8 +289,12 @@ func GetTaskDefinition(integration db.Integration, payload []byte, r *http.Reque
289289
if err != nil {
290290
return
291291
}
292+
}
292293

293-
for k, v := range extractedEnvResults {
294+
// Add extracted environment variables only if they don't conflict with
295+
// existing task definition variables (task definition has higher priority)
296+
for k, v := range extractedEnvResults {
297+
if _, exists := env[k]; !exists {
294298
env[k] = v
295299
}
296300
}

‎api/integration_test.go

Lines changed: 260 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,11 @@
11
package api
22

33
import (
4-
"github.com/semaphoreui/semaphore/db"
4+
"encoding/json"
55
"net/http"
66
"testing"
7+
8+
"github.com/semaphoreui/semaphore/db"
79
)
810

911
func TestIntegrationMatch(t *testing.T) {
@@ -24,3 +26,260 @@ func TestIntegrationMatch(t *testing.T) {
2426
t.Fatal()
2527
}
2628
}
29+
30+
func TestGetTaskDefinitionWithExtractedEnvValues(t *testing.T) {
31+
// Test case 1: Empty environment should still include extracted values
32+
integration := db.Integration{
33+
ID: 1,
34+
ProjectID: 1,
35+
TemplateID: 1,
36+
}
37+
38+
// Create test payload
39+
payload := []byte("{\"branch\": \"main\", \"commit\": \"abc123\"}")
40+
41+
// Create test request with headers
42+
req, _ := http.NewRequest("POST", "/webhook", nil)
43+
req.Header.Set("X-GitHub-Event", "push")
44+
45+
// Mock extracted environment values (this would normally come from database)
46+
envValues := []db.IntegrationExtractValue{
47+
{
48+
Variable: "BRANCH_NAME",
49+
ValueSource: db.IntegrationExtractBodyValue,
50+
BodyDataType: db.IntegrationBodyDataJSON,
51+
Key: "branch",
52+
VariableType: db.IntegrationVariableEnvironment,
53+
},
54+
{
55+
Variable: "COMMIT_HASH",
56+
ValueSource: db.IntegrationExtractBodyValue,
57+
BodyDataType: db.IntegrationBodyDataJSON,
58+
Key: "commit",
59+
VariableType: db.IntegrationVariableEnvironment,
60+
},
61+
{
62+
Variable: "EVENT_TYPE",
63+
ValueSource: db.IntegrationExtractHeaderValue,
64+
Key: "X-GitHub-Event",
65+
VariableType: db.IntegrationVariableEnvironment,
66+
},
67+
}
68+
69+
// Test Extract function directly first
70+
extractedEnvResults := Extract(envValues, req, payload)
71+
72+
if extractedEnvResults["BRANCH_NAME"] != "main" {
73+
t.Errorf("Expected BRANCH_NAME to be 'main', got '%s'", extractedEnvResults["BRANCH_NAME"])
74+
}
75+
if extractedEnvResults["COMMIT_HASH"] != "abc123" {
76+
t.Errorf("Expected COMMIT_HASH to be 'abc123', got '%s'", extractedEnvResults["COMMIT_HASH"])
77+
}
78+
if extractedEnvResults["EVENT_TYPE"] != "push" {
79+
t.Errorf("Expected EVENT_TYPE to be 'push', got '%s'", extractedEnvResults["EVENT_TYPE"])
80+
}
81+
82+
// Test case 1: Empty environment should include extracted values (FIXED behavior)
83+
taskDef1 := db.Task{
84+
ProjectID: 1,
85+
TemplateID: 1,
86+
Environment: "", // Empty environment
87+
Params: make(db.MapStringAnyField),
88+
}
89+
taskDef1.IntegrationID = &integration.ID
90+
91+
// Simulate the FIXED logic from GetTaskDefinition
92+
env1 := make(map[string]any)
93+
94+
if taskDef1.Environment != "" {
95+
json.Unmarshal([]byte(taskDef1.Environment), &env1)
96+
}
97+
98+
// Add extracted environment variables only if they don't conflict with
99+
// existing task definition variables (task definition has higher priority)
100+
for k, v := range extractedEnvResults {
101+
if _, exists := env1[k]; !exists {
102+
env1[k] = v
103+
}
104+
}
105+
106+
envStr1, _ := json.Marshal(env1)
107+
taskDef1.Environment = string(envStr1)
108+
109+
// Verify that extracted values ARE now in the environment
110+
var envCheck1 map[string]any
111+
json.Unmarshal([]byte(taskDef1.Environment), &envCheck1)
112+
113+
if envCheck1["BRANCH_NAME"] != "main" {
114+
t.Errorf("Expected BRANCH_NAME to be 'main' in environment, got '%v'", envCheck1["BRANCH_NAME"])
115+
}
116+
if envCheck1["COMMIT_HASH"] != "abc123" {
117+
t.Errorf("Expected COMMIT_HASH to be 'abc123' in environment, got '%v'", envCheck1["COMMIT_HASH"])
118+
}
119+
if envCheck1["EVENT_TYPE"] != "push" {
120+
t.Errorf("Expected EVENT_TYPE to be 'push' in environment, got '%v'", envCheck1["EVENT_TYPE"])
121+
}
122+
123+
// Test case 2: Existing environment should merge with extracted values
124+
taskDef2 := db.Task{
125+
ProjectID: 1,
126+
TemplateID: 1,
127+
Environment: `{"EXISTING_VAR": "existing_value"}`, // Existing environment
128+
Params: make(db.MapStringAnyField),
129+
}
130+
taskDef2.IntegrationID = &integration.ID
131+
132+
env2 := make(map[string]any)
133+
134+
if taskDef2.Environment != "" {
135+
json.Unmarshal([]byte(taskDef2.Environment), &env2)
136+
}
137+
138+
// Add extracted environment variables only if they don't conflict with
139+
// existing task definition variables (task definition has higher priority)
140+
for k, v := range extractedEnvResults {
141+
if _, exists := env2[k]; !exists {
142+
env2[k] = v
143+
}
144+
}
145+
146+
envStr2, _ := json.Marshal(env2)
147+
taskDef2.Environment = string(envStr2)
148+
149+
// Verify that both existing and extracted values are in the environment
150+
var envCheck2 map[string]any
151+
json.Unmarshal([]byte(taskDef2.Environment), &envCheck2)
152+
153+
if envCheck2["EXISTING_VAR"] != "existing_value" {
154+
t.Errorf("Expected EXISTING_VAR to be 'existing_value' in environment, got '%v'", envCheck2["EXISTING_VAR"])
155+
}
156+
if envCheck2["BRANCH_NAME"] != "main" {
157+
t.Errorf("Expected BRANCH_NAME to be 'main' in environment, got '%v'", envCheck2["BRANCH_NAME"])
158+
}
159+
if envCheck2["COMMIT_HASH"] != "abc123" {
160+
t.Errorf("Expected COMMIT_HASH to be 'abc123' in environment, got '%v'", envCheck2["COMMIT_HASH"])
161+
}
162+
if envCheck2["EVENT_TYPE"] != "push" {
163+
t.Errorf("Expected EVENT_TYPE to be 'push' in environment, got '%v'", envCheck2["EVENT_TYPE"])
164+
}
165+
166+
// Test case 3: Task definition values should have priority over extracted values
167+
taskDef3 := db.Task{
168+
ProjectID: 1,
169+
TemplateID: 1,
170+
Environment: `{"BRANCH_NAME": "production", "EXISTING_VAR": "from_task"}`, // Conflicts with extracted BRANCH_NAME
171+
Params: make(db.MapStringAnyField),
172+
}
173+
taskDef3.IntegrationID = &integration.ID
174+
175+
env3 := make(map[string]any)
176+
177+
if taskDef3.Environment != "" {
178+
json.Unmarshal([]byte(taskDef3.Environment), &env3)
179+
}
180+
181+
// Add extracted environment variables only if they don't conflict with
182+
// existing task definition variables (task definition has higher priority)
183+
for k, v := range extractedEnvResults {
184+
if _, exists := env3[k]; !exists {
185+
env3[k] = v
186+
}
187+
}
188+
189+
envStr3, _ := json.Marshal(env3)
190+
taskDef3.Environment = string(envStr3)
191+
192+
// Verify that task definition values take precedence over extracted values
193+
var envCheck3 map[string]any
194+
json.Unmarshal([]byte(taskDef3.Environment), &envCheck3)
195+
196+
// BRANCH_NAME should remain "production" from task definition, not "main" from extracted
197+
if envCheck3["BRANCH_NAME"] != "production" {
198+
t.Errorf("Expected BRANCH_NAME to be 'production' (task definition priority), got '%v'", envCheck3["BRANCH_NAME"])
199+
}
200+
// EXISTING_VAR should remain from task definition
201+
if envCheck3["EXISTING_VAR"] != "from_task" {
202+
t.Errorf("Expected EXISTING_VAR to be 'from_task', got '%v'", envCheck3["EXISTING_VAR"])
203+
}
204+
// Non-conflicting extracted values should still be added
205+
if envCheck3["COMMIT_HASH"] != "abc123" {
206+
t.Errorf("Expected COMMIT_HASH to be 'abc123' in environment, got '%v'", envCheck3["COMMIT_HASH"])
207+
}
208+
if envCheck3["EVENT_TYPE"] != "push" {
209+
t.Errorf("Expected EVENT_TYPE to be 'push' in environment, got '%v'", envCheck3["EVENT_TYPE"])
210+
}
211+
}
212+
213+
// Test the Extract function to ensure it works correctly for both body and header extraction
214+
func TestExtractBodyAndHeaderValues(t *testing.T) {
215+
// Create test payload with nested JSON
216+
payload := []byte(`{"repository": {"name": "test-repo"}, "ref": "refs/heads/main", "pusher": {"name": "johndoe"}}`)
217+
218+
// Create test request with headers
219+
req, _ := http.NewRequest("POST", "/webhook", nil)
220+
req.Header.Set("X-GitHub-Event", "push")
221+
req.Header.Set("X-GitHub-Delivery", "12345")
222+
223+
// Test various extraction scenarios
224+
extractValues := []db.IntegrationExtractValue{
225+
{
226+
Variable: "REPO_NAME",
227+
ValueSource: db.IntegrationExtractBodyValue,
228+
BodyDataType: db.IntegrationBodyDataJSON,
229+
Key: "repository.name",
230+
},
231+
{
232+
Variable: "GIT_REF",
233+
ValueSource: db.IntegrationExtractBodyValue,
234+
BodyDataType: db.IntegrationBodyDataJSON,
235+
Key: "ref",
236+
},
237+
{
238+
Variable: "PUSHER_NAME",
239+
ValueSource: db.IntegrationExtractBodyValue,
240+
BodyDataType: db.IntegrationBodyDataJSON,
241+
Key: "pusher.name",
242+
},
243+
{
244+
Variable: "GITHUB_EVENT",
245+
ValueSource: db.IntegrationExtractHeaderValue,
246+
Key: "X-GitHub-Event",
247+
},
248+
{
249+
Variable: "GITHUB_DELIVERY",
250+
ValueSource: db.IntegrationExtractHeaderValue,
251+
Key: "X-GitHub-Delivery",
252+
},
253+
{
254+
Variable: "FULL_PAYLOAD",
255+
ValueSource: db.IntegrationExtractBodyValue,
256+
BodyDataType: db.IntegrationBodyDataString,
257+
},
258+
}
259+
260+
result := Extract(extractValues, req, payload)
261+
262+
// Verify body JSON extractions
263+
if result["REPO_NAME"] != "test-repo" {
264+
t.Errorf("Expected REPO_NAME to be 'test-repo', got '%s'", result["REPO_NAME"])
265+
}
266+
if result["GIT_REF"] != "refs/heads/main" {
267+
t.Errorf("Expected GIT_REF to be 'refs/heads/main', got '%s'", result["GIT_REF"])
268+
}
269+
if result["PUSHER_NAME"] != "johndoe" {
270+
t.Errorf("Expected PUSHER_NAME to be 'johndoe', got '%s'", result["PUSHER_NAME"])
271+
}
272+
273+
// Verify header extractions
274+
if result["GITHUB_EVENT"] != "push" {
275+
t.Errorf("Expected GITHUB_EVENT to be 'push', got '%s'", result["GITHUB_EVENT"])
276+
}
277+
if result["GITHUB_DELIVERY"] != "12345" {
278+
t.Errorf("Expected GITHUB_DELIVERY to be '12345', got '%s'", result["GITHUB_DELIVERY"])
279+
}
280+
281+
// Verify string body extraction
282+
if result["FULL_PAYLOAD"] != string(payload) {
283+
t.Errorf("Expected FULL_PAYLOAD to match original payload")
284+
}
285+
}

‎web/public/test.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
test

0 commit comments

Comments
 (0)