Skip to content

Commit f800d50

Browse files
chengjoeychengjoey
committed
fix task parameter value substitution error due to propagateParams
1 parent 12457e2 commit f800d50

File tree

2 files changed

+295
-23
lines changed

2 files changed

+295
-23
lines changed

‎pkg/reconciler/pipelinerun/resources/apply.go

+41-23
Original file line numberDiff line numberDiff line change
@@ -236,7 +236,7 @@ func ApplyReplacements(p *v1beta1.PipelineSpec, replacements map[string]string,
236236
if p.Tasks[i].TaskRef != nil && p.Tasks[i].TaskRef.Params != nil {
237237
p.Tasks[i].TaskRef.Params = replaceParamValues(p.Tasks[i].TaskRef.Params, replacements, arrayReplacements, objectReplacements)
238238
}
239-
p.Tasks[i], replacements, arrayReplacements, objectReplacements = propagateParams(p.Tasks[i], replacements, arrayReplacements, objectReplacements)
239+
p.Tasks[i] = propagateParams(p.Tasks[i], replacements, arrayReplacements, objectReplacements)
240240
}
241241

242242
for i := range p.Finally {
@@ -251,38 +251,56 @@ func ApplyReplacements(p *v1beta1.PipelineSpec, replacements map[string]string,
251251
if p.Finally[i].TaskRef != nil && p.Finally[i].TaskRef.Params != nil {
252252
p.Finally[i].TaskRef.Params = replaceParamValues(p.Finally[i].TaskRef.Params, replacements, arrayReplacements, objectReplacements)
253253
}
254-
p.Finally[i], replacements, arrayReplacements, objectReplacements = propagateParams(p.Finally[i], replacements, arrayReplacements, objectReplacements)
254+
p.Finally[i] = propagateParams(p.Finally[i], replacements, arrayReplacements, objectReplacements)
255255
}
256256

257257
return p
258258
}
259259

260-
func propagateParams(t v1beta1.PipelineTask, replacements map[string]string, arrayReplacements map[string][]string, objectReplacements map[string]map[string]string) (v1beta1.PipelineTask, map[string]string, map[string][]string, map[string]map[string]string) {
261-
if t.TaskSpec != nil {
262-
// check if there are task parameters defined that match the params at pipeline level
263-
if len(t.Params) > 0 {
264-
for _, par := range t.Params {
265-
for _, pattern := range paramPatterns {
266-
checkName := fmt.Sprintf(pattern, par.Name)
267-
// Scoping. Task Params will replace Pipeline Params
268-
if _, ok := replacements[checkName]; ok {
269-
replacements[checkName] = par.Value.StringVal
270-
}
271-
if _, ok := arrayReplacements[checkName]; ok {
272-
arrayReplacements[checkName] = par.Value.ArrayVal
273-
}
274-
if _, ok := objectReplacements[checkName]; ok {
275-
objectReplacements[checkName] = par.Value.ObjectVal
276-
for k, v := range par.Value.ObjectVal {
277-
replacements[fmt.Sprintf(objectIndividualVariablePattern, par.Name, k)] = v
278-
}
260+
// propagateParams returns a Pipeline Task spec that is the same as the input Pipeline Task spec, but with
261+
// all parameter replacements from `stringReplacements`, `arrayReplacements`, and `objectReplacements` substituted.
262+
// It does not modify `stringReplacements`, `arrayReplacements`, or `objectReplacements`.
263+
func propagateParams(t v1beta1.PipelineTask, stringReplacements map[string]string, arrayReplacements map[string][]string, objectReplacements map[string]map[string]string) v1beta1.PipelineTask {
264+
if t.TaskSpec == nil {
265+
return t
266+
}
267+
// check if there are task parameters defined that match the params at pipeline level
268+
if len(t.Params) > 0 {
269+
stringReplacementsDup := make(map[string]string)
270+
arrayReplacementsDup := make(map[string][]string)
271+
objectReplacementsDup := make(map[string]map[string]string)
272+
for k, v := range stringReplacements {
273+
stringReplacementsDup[k] = v
274+
}
275+
for k, v := range arrayReplacements {
276+
arrayReplacementsDup[k] = v
277+
}
278+
for k, v := range objectReplacements {
279+
objectReplacementsDup[k] = v
280+
}
281+
for _, par := range t.Params {
282+
for _, pattern := range paramPatterns {
283+
checkName := fmt.Sprintf(pattern, par.Name)
284+
// Scoping. Task Params will replace Pipeline Params
285+
if _, ok := stringReplacementsDup[checkName]; ok {
286+
stringReplacementsDup[checkName] = par.Value.StringVal
287+
}
288+
if _, ok := arrayReplacementsDup[checkName]; ok {
289+
arrayReplacementsDup[checkName] = par.Value.ArrayVal
290+
}
291+
if _, ok := objectReplacementsDup[checkName]; ok {
292+
objectReplacementsDup[checkName] = par.Value.ObjectVal
293+
for k, v := range par.Value.ObjectVal {
294+
stringReplacementsDup[fmt.Sprintf(objectIndividualVariablePattern, par.Name, k)] = v
279295
}
280296
}
281297
}
282298
}
283-
t.TaskSpec.TaskSpec = *resources.ApplyReplacements(&t.TaskSpec.TaskSpec, replacements, arrayReplacements)
299+
t.TaskSpec.TaskSpec = *resources.ApplyReplacements(&t.TaskSpec.TaskSpec, stringReplacementsDup, arrayReplacementsDup)
300+
} else {
301+
t.TaskSpec.TaskSpec = *resources.ApplyReplacements(&t.TaskSpec.TaskSpec, stringReplacements, arrayReplacements)
284302
}
285-
return t, replacements, arrayReplacements, objectReplacements
303+
return t
286304
}
287305

288306
func replaceParamValues(params []v1beta1.Param, stringReplacements map[string]string, arrayReplacements map[string][]string, objectReplacements map[string]map[string]string) []v1beta1.Param {

‎pkg/reconciler/pipelinerun/resources/apply_test.go

+254
Original file line numberDiff line numberDiff line change
@@ -1450,6 +1450,260 @@ func TestApplyParameters(t *testing.T) {
14501450
}},
14511451
},
14521452
},
1453+
{
1454+
name: "tasks with the same parameter name but referencing different values",
1455+
original: v1beta1.PipelineSpec{
1456+
Params: []v1beta1.ParamSpec{
1457+
{
1458+
Name: "param1",
1459+
Default: &v1beta1.ParamValue{
1460+
Type: v1beta1.ParamTypeString,
1461+
StringVal: "a",
1462+
},
1463+
Type: v1beta1.ParamTypeString,
1464+
},
1465+
},
1466+
Tasks: []v1beta1.PipelineTask{
1467+
{
1468+
Name: "previous-task-has-result",
1469+
},
1470+
{
1471+
Name: "print-msg",
1472+
TaskSpec: &v1beta1.EmbeddedTask{
1473+
TaskSpec: v1beta1.TaskSpec{
1474+
Params: []v1beta1.ParamSpec{
1475+
{
1476+
Name: "param1",
1477+
Type: v1beta1.ParamTypeString,
1478+
},
1479+
},
1480+
},
1481+
},
1482+
Params: []v1beta1.Param{
1483+
{
1484+
Name: "param1",
1485+
Value: v1beta1.ParamValue{
1486+
Type: v1beta1.ParamTypeString,
1487+
StringVal: "$(tasks.previous-task-has-result.results.Output)",
1488+
},
1489+
},
1490+
},
1491+
},
1492+
{
1493+
Name: "print-msg-2",
1494+
TaskSpec: &v1beta1.EmbeddedTask{
1495+
TaskSpec: v1beta1.TaskSpec{
1496+
Params: []v1beta1.ParamSpec{
1497+
{
1498+
Name: "param1",
1499+
Type: v1beta1.ParamTypeString,
1500+
},
1501+
},
1502+
},
1503+
},
1504+
Params: []v1beta1.Param{
1505+
{
1506+
Name: "param1",
1507+
Value: v1beta1.ParamValue{
1508+
Type: v1beta1.ParamTypeString,
1509+
StringVal: "$(params.param1)",
1510+
},
1511+
},
1512+
},
1513+
},
1514+
},
1515+
},
1516+
expected: v1beta1.PipelineSpec{
1517+
Params: []v1beta1.ParamSpec{
1518+
{
1519+
Name: "param1",
1520+
Default: &v1beta1.ParamValue{
1521+
Type: v1beta1.ParamTypeString,
1522+
StringVal: "a",
1523+
},
1524+
Type: v1beta1.ParamTypeString,
1525+
},
1526+
},
1527+
Tasks: []v1beta1.PipelineTask{
1528+
{
1529+
Name: "previous-task-has-result",
1530+
},
1531+
{
1532+
Name: "print-msg",
1533+
TaskSpec: &v1beta1.EmbeddedTask{
1534+
TaskSpec: v1beta1.TaskSpec{
1535+
Params: []v1beta1.ParamSpec{
1536+
{
1537+
Name: "param1",
1538+
Type: v1beta1.ParamTypeString,
1539+
},
1540+
},
1541+
},
1542+
},
1543+
Params: []v1beta1.Param{
1544+
{
1545+
Name: "param1",
1546+
Value: v1beta1.ParamValue{
1547+
Type: v1beta1.ParamTypeString,
1548+
StringVal: "$(tasks.previous-task-has-result.results.Output)",
1549+
},
1550+
},
1551+
},
1552+
},
1553+
{
1554+
Name: "print-msg-2",
1555+
TaskSpec: &v1beta1.EmbeddedTask{
1556+
TaskSpec: v1beta1.TaskSpec{
1557+
Params: []v1beta1.ParamSpec{
1558+
{
1559+
Name: "param1",
1560+
Type: v1beta1.ParamTypeString,
1561+
},
1562+
},
1563+
},
1564+
},
1565+
Params: []v1beta1.Param{
1566+
{
1567+
Name: "param1",
1568+
Value: v1beta1.ParamValue{
1569+
Type: v1beta1.ParamTypeString,
1570+
StringVal: "a",
1571+
},
1572+
},
1573+
},
1574+
},
1575+
},
1576+
},
1577+
},
1578+
{
1579+
name: "finally tasks with the same parameter name but referencing different values",
1580+
original: v1beta1.PipelineSpec{
1581+
Params: []v1beta1.ParamSpec{
1582+
{
1583+
Name: "param1",
1584+
Default: &v1beta1.ParamValue{
1585+
Type: v1beta1.ParamTypeString,
1586+
StringVal: "a",
1587+
},
1588+
Type: v1beta1.ParamTypeString,
1589+
},
1590+
},
1591+
Tasks: []v1beta1.PipelineTask{
1592+
{
1593+
Name: "previous-task-has-result",
1594+
},
1595+
},
1596+
Finally: []v1beta1.PipelineTask{
1597+
{
1598+
Name: "print-msg",
1599+
TaskSpec: &v1beta1.EmbeddedTask{
1600+
TaskSpec: v1beta1.TaskSpec{
1601+
Params: []v1beta1.ParamSpec{
1602+
{
1603+
Name: "param1",
1604+
Type: v1beta1.ParamTypeString,
1605+
},
1606+
},
1607+
},
1608+
},
1609+
Params: []v1beta1.Param{
1610+
{
1611+
Name: "param1",
1612+
Value: v1beta1.ParamValue{
1613+
Type: v1beta1.ParamTypeString,
1614+
StringVal: "$(tasks.previous-task-has-result.results.Output)",
1615+
},
1616+
},
1617+
},
1618+
},
1619+
{
1620+
Name: "print-msg-2",
1621+
TaskSpec: &v1beta1.EmbeddedTask{
1622+
TaskSpec: v1beta1.TaskSpec{
1623+
Params: []v1beta1.ParamSpec{
1624+
{
1625+
Name: "param1",
1626+
Type: v1beta1.ParamTypeString,
1627+
},
1628+
},
1629+
},
1630+
},
1631+
Params: []v1beta1.Param{
1632+
{
1633+
Name: "param1",
1634+
Value: v1beta1.ParamValue{
1635+
Type: v1beta1.ParamTypeString,
1636+
StringVal: "$(params.param1)",
1637+
},
1638+
},
1639+
},
1640+
},
1641+
},
1642+
},
1643+
expected: v1beta1.PipelineSpec{
1644+
Params: []v1beta1.ParamSpec{
1645+
{
1646+
Name: "param1",
1647+
Default: &v1beta1.ParamValue{
1648+
Type: v1beta1.ParamTypeString,
1649+
StringVal: "a",
1650+
},
1651+
Type: v1beta1.ParamTypeString,
1652+
},
1653+
},
1654+
Tasks: []v1beta1.PipelineTask{
1655+
{
1656+
Name: "previous-task-has-result",
1657+
},
1658+
},
1659+
Finally: []v1beta1.PipelineTask{
1660+
{
1661+
Name: "print-msg",
1662+
TaskSpec: &v1beta1.EmbeddedTask{
1663+
TaskSpec: v1beta1.TaskSpec{
1664+
Params: []v1beta1.ParamSpec{
1665+
{
1666+
Name: "param1",
1667+
Type: v1beta1.ParamTypeString,
1668+
},
1669+
},
1670+
},
1671+
},
1672+
Params: []v1beta1.Param{
1673+
{
1674+
Name: "param1",
1675+
Value: v1beta1.ParamValue{
1676+
Type: v1beta1.ParamTypeString,
1677+
StringVal: "$(tasks.previous-task-has-result.results.Output)",
1678+
},
1679+
},
1680+
},
1681+
},
1682+
{
1683+
Name: "print-msg-2",
1684+
TaskSpec: &v1beta1.EmbeddedTask{
1685+
TaskSpec: v1beta1.TaskSpec{
1686+
Params: []v1beta1.ParamSpec{
1687+
{
1688+
Name: "param1",
1689+
Type: v1beta1.ParamTypeString,
1690+
},
1691+
},
1692+
},
1693+
},
1694+
Params: []v1beta1.Param{
1695+
{
1696+
Name: "param1",
1697+
Value: v1beta1.ParamValue{
1698+
Type: v1beta1.ParamTypeString,
1699+
StringVal: "a",
1700+
},
1701+
},
1702+
},
1703+
},
1704+
},
1705+
},
1706+
},
14531707
} {
14541708
tt := tt // capture range variable
14551709
ctx := context.Background()

0 commit comments

Comments
 (0)