Skip to content

fix: don't treat $FOO_BAR as a placeholder for substitution FOO - #810

Open
NopeCharon wants to merge 1 commit into
project-stacker:mainfrom
NopeCharon:fix/substitution-placeholder-prefix-match
Open

NopeCharon wants to merge 1 commit into
project-stacker:mainfrom
NopeCharon:fix/substitution-placeholder-prefix-match

Conversation

@NopeCharon

Copy link
Copy Markdown

What type of PR is this?

bug

Which issue does this PR fix:

Fixes #809

What does this PR do / Why do we need it:

When a substitution KEY=value is provided, substitute() rejects the stacker file if it contains the shell-style placeholder $KEY. The check was strings.Contains(content, "$KEY"), which also matches any variable whose name just starts with KEY. As a result, stacker build --substitute VERSION=1.0 failed on a run section that uses $VERSION_ID from /etc/os-release:

error "VERSION=1.0" was provided as a substitution and unsupported placeholder "$VERSION" was found. ...
1 instances of unsupported placeholders found. Review log for how to update.

This PR reports $KEY only when it is not followed by another variable-name character ([A-Za-z0-9_]), using a regexp built with regexp.QuoteMeta. The ${KEY} and ${KEY:...} checks already require an exact key match, so they are unchanged. The substitution syntax docs now mention this behavior.

If an issue # is not available please add repro steps and logs showing the issue:

See #809.

Testing done on this change:

Extended TestSubstitute:

  • $VERSION_ID $VERSIONS ${VERSION_ID} ${VERSIONS:-1} ${{VERSION}} with VERSION=1.0 now substitutes successfully and leaves the shell variables untouched.
  • $VERSION, $VERSION/bin, $VERSION-rc1 and "$VERSION" are still rejected.

Without the fix, the new case fails:

--- FAIL: TestSubstitute (0.00s)
    types_test.go:116: failed substitution: 1 instances of unsupported placeholders found. Review log for how to update.

With the fix, go test ./pkg/types/ passes (on linux/arm64, using the Makefile's go-test build tags):

--- PASS: TestSubstitute (0.00s)
...
ok  	stackerbuild.io/stacker/pkg/types	0.017s

Automation added to e2e:

No. The behavior is covered by the TestSubstitute unit test.

Will this break upgrades or downgrades?

No.

Does this PR introduce any user-facing change?:

Shell variables whose names start with a provided substitution key (e.g. `$VERSION_ID` with `--substitute VERSION=...`) no longer cause an "unsupported placeholder" error.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

When a substitution KEY=value is provided, substitute() rejects the
stacker file if it contains the shell style placeholder $KEY, to point
users at the supported ${{KEY}} syntax. The check used
strings.Contains(content, "$KEY"), which also matches any variable
whose name merely starts with KEY. For example, building with
--substitute VERSION=1.0 failed with an "unsupported placeholder"
error for a run section using $VERSION_ID from /etc/os-release.

Only report $KEY when it is not followed by another variable name
character ([A-Za-z0-9_]). The ${KEY} checks already require an exact
key match and are unchanged.

Add test cases for variables sharing the key as a prefix, and for $KEY
followed by non-name characters, which must still be rejected. Mention
the behavior in the substitution syntax documentation.

Fixes project-stacker#809

Signed-off-by: NopeCharon <115794254+NopeCharon@users.noreply.github.com>
@codecov

codecov Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 12.94%. Comparing base (715bf05) to head (977b312).

Files with missing lines Patch % Lines
pkg/types/stackerfile.go 85.00% 2 Missing and 1 partial ⚠️

❗ There is a different number of reports uploaded between BASE (715bf05) and HEAD (977b312). Click for more details.

HEAD has 37 uploads less than BASE
Flag BASE (715bf05) HEAD (977b312)
40 3
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #810       +/-   ##
===========================================
- Coverage   55.06%   12.94%   -42.12%     
===========================================
  Files          55       55               
  Lines        5911     5924       +13     
===========================================
- Hits         3255      767     -2488     
- Misses       2085     5030     +2945     
+ Partials      571      127      -444     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: substitution key wrongly rejects shell variables that start with the same name ($VERSION_ID for VERSION)

1 participant