fix: add a special case for item variable during global expression replacement#15033
Conversation
91fc59e to
98df7eb
Compare
…replacement
* The root cause of the issue was from SubstituteParams, where the
`globalReplacedTmplStr` would have already lost the `={{sprig.func(item)}}`
expression which became an empty string. This meant the later `processItem`
function had no expression to expand.
* This seems to have come from a change in sprig which in turn came from
the `expr` library upgrade from 1.16 to 1.17 in 9b7c0c
* We now have a special case to _not_ attempt to run the `expr` program
as an unresolved `item` in a sprig string function now comes back as an
empty string, instead of `nil`.
Signed-off-by: Elliot Gunton <elliotgunton@gmail.com>
Signed-off-by: Elliot Gunton <elliotgunton@gmail.com>
Signed-off-by: Elliot Gunton <elliotgunton@gmail.com>
1a15f08 to
af89a1c
Compare
Signed-off-by: Elliot Gunton <elliotgunton@gmail.com>
af89a1c to
44480a2
Compare
Signed-off-by: Elliot Gunton <elliotgunton@gmail.com>
Signed-off-by: Elliot Gunton <elliotgunton@gmail.com>
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue where the item variable in sprig string functions (like sprig.upper(item)) was being incorrectly resolved to an empty string during global expression replacement, causing withItems loops to fail. The fix adds special handling for the item variable alongside existing special cases for retries, lastRetry.*, and workflow.* variables.
- Refactored
anyVarNotInEnvto check against a centralized list of variables and return the missing variable name - Consolidated multiple conditional checks into a single unified check with a
variablesToChecklist - Removed unused
hasRetriesfunction and improved test coverage
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| util/template/expression_template.go | Refactored variable checking logic to use a centralized list and unified handling for unresolved variables including item |
| util/template/expression_template_test.go | Renamed and expanded test from Test_hasRetries to Test_hasVariableInExpression with additional test cases for item variable |
| test/e2e/expr_lang_test.go | Added e2e regression test for issue #15008 and fixed typo in existing test suite name |
Comments suppressed due to low confidence (1)
test/e2e/expr_lang_test.go:110
- Corrected spelling from 'TestExprLangSSuite' to 'TestExprLangSuite' (removed duplicate 'S').
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Elliot Gunton <elliotgunton@gmail.com>
|
❌ Cherry-pick failed for 3.6. Please check the workflow logs for details. |
|
❌ Cherry-pick failed for 3.7. Please check the workflow logs for details. |
…replacement (cherry-pick argoproj#15033 for 3.7) (argoproj#15036) Signed-off-by: Elliot Gunton <elliotgunton@gmail.com> Signed-off-by: Alan Clucas <alan@clucas.org> Co-authored-by: Elliot Gunton <elliotgunton@gmail.com>
…replacement (argoproj#15033) Signed-off-by: Elliot Gunton <elliotgunton@gmail.com>
Fixes #15008
Motivation
globalReplacedTmplStrwould have already lost the={{sprig.func(item)}}expression which became an empty string. This meant the laterprocessItemfunction had no expression to expand.exprlibrary upgrade from 1.16 to 1.17 in 9b7c0c4Modifications
exprprogram as an unresolveditemin a sprig string function now comes back as an empty string, instead ofnil.anyVarNotInEnvto return the varname that is missing as a reference to a string, ornil.hasRetriesand refactored and expanded the unit tests where it was usedVerification