project-stacker / stacker

Build OCI images natively from a declarative format
https://stackerbuild.io
Apache License 2.0
208 stars 34 forks source link

2024.02.27/main/stricter substitutions #598

Closed mikemccracken closed 8 months ago

mikemccracken commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

"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[bot] commented 8 months ago

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 :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mikemccracken commented 8 months ago

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 commented 8 months ago

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