[mikemccracken]

What type of PR is this?

This is a breaking change to the substitution syntax, which was bad and should be made good.

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

This makes it an error to have placeholders that are valid shell like $PATH or ${PATH} if you have provided a substitution for PATH to stacker via --substitute PATH=why:oh:why.

Those placeholders are too easy to confuse with a valid shell variable, and stacker can't tell if you intended to provide a value for them but typoed it - and you can't tell by looking at it if it is intended to be a stacker sub or just a shell var.

So they are bad and should be made good by becoming fatal errors.

The documented behavior in docs was not tested fully, and was in fact wrong. Now it's right.

This change is confined to one function that has go unit tests, so I just updated those and the one place where our tests were using $PLAIN syntax.

Will this break upgrades or downgrades?

Possibly. If people were doing confusing things like --substitute PATH=ouch, then their builds will fail with a helpful message about how to fix it.

Does this PR introduce any user-facing change?:

Substitution placeholders in stacker files with no braces or single braces are no longer supported, and an error will be printed to explain how to replace them. 

all valid shell variables with no braces or single braces are still fine, the change is that now if you provide a substitution `--substitute FOO=val`, then use `$FOO` or `${FOO}` in your stacker file, that will be a fatal error. You must use `${{FOO}}` instead to have stacker replace it with the provided substitution.

An example of the new error output is:

2024/02/27 16:36:03 error "A=B" was provided as a substitution and unsupported placeholder "${A}" was found. Replace "${A}" with "${{A}}" to use the substitution.
 isStacker=&{}

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

[mikemccracken]

Note that there are two commits. The first commit removes support for the other sub styles, but just warns when they are found to be shadowed. The second commit upgrades them to errors, which I think is the right way to go, but am open to discussion.

[rchincha]

"BUSYBOX_OCI=/home/runner/work/stacker/stacker/test/busybox:latest" was provided as a substitution and unsupported placeholder "$BUSYBOX_OCI" was found. Replace "$BUSYBOX_OCI" with "${{BUSYBOX_OCI}}" to use the substitution.

^ breaking test.

[codecov]

Codecov Report

Attention: Patch coverage is 93.47826% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 57.38%. Comparing base (da5e3c8) to head (0b8f4cc).
Report is 2 commits behind head on main.

Files	Patch %	Lines
pkg/types/stackerfile.go	93.47%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #598      +/-   ##
==========================================
+ Coverage   57.15%   57.38%   +0.23%     
==========================================
  Files          64       64              
  Lines        7525     7566      +41     
==========================================
+ Hits         4301     4342      +41     
  Misses       2481     2481              
  Partials      743      743              

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

[rchincha]

lgtm

Have to make changes to our official docs.

[mikemccracken]

I noticed that I hadn't limited the test changes to the last commit, so I just reworked the last two.
The test changes should work even without the other commits, so we could even consider putting them first, if we want every commit to be able to pass CI on its own

[hallyn]

Thanks.

[hallyn]

Did you want to squash these (to combine the commit msgs yourself), or just squash-and-merge?