Skip to content

Commit e87eda6

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

File tree

2 files changed

+279
-23
lines changed

2 files changed

+279
-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

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

0 commit comments

Comments
 (0)