Make string variable interpolation deterministic, and single-pass.

[dlorenc]

Changes

Make string variable interpolation deterministic, and single-pass.

Right now we replace string variables one at a time, in an order governed by
iteration through a map[string]string. This has two implications:

• We may expand variables twice. If one variable expands to another variable, that one may be expanded. And so on.
• Whether or not this secondary expansion occurs (and even how it occurs!) can vary from run to run.

This commit changes to replace everything at once, using the strings.Replacer package.
It also adds a test to ensure replacements are stable and non-recursive.

See #2093

Note, we could go the other way this PR and instead embrace nested expansions. This one is simply my preference based on some reasons outlined in the issue.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

• Includes tests (if functionality changed/added)
• Commit messages follow commit message best practices
• Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

• If you are adding a new binary/image to the cmd dir, please update
  the release Task to build and release this image.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

The variable interpolation algorithm has been changed. Nested expansion is no longer supported.
If you use replacements ( $(...) syntax) in your TaskRuns or PipelineRuns that resolve to secondary replacements, these will no longer work.

The algorithm is now deterministic and will result in the same expansion every run.

[dlorenc]

Note that I'm not sure this is the best way to solve this issue. I could also easily be persuaded to correctly handle nested expansions, if we want to go that way. The important thing is that it's easy to describe and deterministic.

[dlorenc]

/kind bug

[dlorenc]

I'm also not sure how much of a "breaking change" this is. Nested expansions were never documented as working, and only worked sometimes. That said, if you were using them and got lucky, this would break you.

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/substitution/substitution.go	58.8%	60.7%	1.9

[dlorenc]

/test pull-tekton-pipeline-integration-tests

Not seeing these failures locally, retrying

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/substitution/substitution.go	58.8%	60.4%	1.6

[dlorenc]

Now with fewer regular expressions, and more passing tests!

[vdemeester]

/lgtm

[dlorenc]

/hold I'm not actually sure if this is an API change or not and would like to discuss it a bit before merging.

[dlorenc]

/hold

[dlorenc]

cc @skaegi who has ideas as discussed in the community meeting

[skaegi]

I'm not at all opposed to making it single-pass for now! The current way we have it just leads to confusion.

With this fix in place, in the future we can add ordered deterministic param expansion without breaking backwards compatibility.

[dlorenc]

With this fix in place, in the future we can add ordered deterministic param expansion without breaking backwards compatibility.

Ah, makes sense. Thanks for clarifying!

[bobcatfish]

I'm not actually sure if this is an API change or not and would like to discuss it a bit before merging.

I'm not sure either! I guess it depends on whether you view it as a bug or not ( i think @imjasonh has some kind of rule about that ). I think it is a bug, however it's also possible some folks are relying on this functionality even so.

My vote would be to go ahead and make the backwards incompatible change; if we wanted to be cautious we could have a release where we try to detect someone is using this functionality and warn them it's going to be removed?

[dlorenc]

/hold cancel

Thanks!

[dlorenc]

For my rationale on why I think this is ok:

• Secondary expansion is definitely unintended behavior, but I wouldn't call it a bug.
• The bug is the non-deterministic nature of the expansion.
• It's not really possible to depend on this, since it's basically random.
• Since it can't be depended on, we can change it.

[imjasonh]

I agree it's a bug and we should feel free to change it.

If we want to support multi-pass interpolation we'll need to have a use case first, then a short TEP. :)

Thanks for catching this Dan!

[afrittoli]

Thanks, nice catch!
/approve

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [afrittoli]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment