Tekton 0.3.1 does not support $() syntax

[afrittoli]

Tekton 0.3.1 is used for release. It does not support the $() syntax
so ${} should be used everywhere.

Changes

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)
• Includes docs (if user facing)
• Commit messages follow commit message best practices

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 and TaskRun 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

Describe any user facing changes here, or delete this block.

Examples of user facing changes:
- API changes
- Bug fixes
- Any changes in behavior

[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

[bobcatfish]

Thanks for jumping on this @afrittoli !

I think in this case we actually want to leave it tho - the $() here is being used by bash, and isn't used for variable interpolation.

/hold

[bobcatfish]

whoa MASSIVE derp on my part, @afrittoli is completely right. obviously I didnt read this carefully so I don't trust myself to lgtm this super quick but at least

/hold cancel

😅

SORRY @afrittoli !!!!

[bobcatfish]

Sorry actually looked at this properly! I was certain this was in the middle of the bash scripts :S

I'm a bit nervous now that our 0.6.0 release (and our nightly releases) might have images with the wrong base images if the contents of that ko config file were written incorrectly. But also I would have expected to see the wrong bucket (literally /workspace/output/bucket/previous/$(inputs.params.versionTag)/) but all the buckets we've released so far look okay - so maybe we avoided releasing with these being incorrrect somehow? Anyway since I can't actually find a problem I'll try to stop worrying 🤣 😅

Thanks again for fixing this @afrittoli and for your patience with my drive-by review where I didn't look carefully at all and made terrible assumptions 🙏

/lgtm

p.s. I looked at the body of the publish-nightly Task applied in the prow cluster and it looks correct? (maybe you fixed it already tho!)

[bobcatfish]

/meow space

[tekton-robot]

@bobcatfish:

In response to this:

/meow space

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[afrittoli]

scripts

Sorry actually looked at this properly! I was certain this was in the middle of the bash scripts :S

I'm a bit nervous now that our 0.6.0 release (and our nightly releases) might have images with the wrong base images if the contents of that ko config file were written incorrectly. But also I would have expected to see the wrong bucket (literally /workspace/output/bucket/previous/$(inputs.params.versionTag)/) but all the buckets we've released so far look okay - so maybe we avoided releasing with these being incorrrect somehow? Anyway since I can't actually find a problem I'll try to stop worrying 🤣 😅

Thanks again for fixing this @afrittoli and for your patience with my drive-by review where I didn't look carefully at all and made terrible assumptions 🙏

/lgtm

p.s. I looked at the body of the publish-nightly Task applied in the prow cluster and it looks correct? (maybe you fixed it already tho!)

@bobcatfish I updated the publish-nightly today because I wanted to test and deploy Jason's patch that adds vendor sources to our docker images (#1338).
On top of #1338 I also fixed the $() to ${} issue since it was complaining about it in the logs - and it was the same issue I had yesterday when running v0.7.0.

I wanted to check the logs from previous releases to see if the issue was there for previous releases too, but all pods were gone so no logs.
Without the base images in ko config, all images will be build on top of distroless, so I fear a few things would not work. We should try installing the last few releases and run integration tests on top so that we know if they work or not.