Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't use variable substitution in script block #1071

Closed

Conversation

zregvart
Copy link
Member

This is considered to be a bad practice[1]. Also breaks trusted artifacts due to the script being reformatted and an extra space added around the parentheses.

[1] https://tekton.dev/docs/pipelines/tasks/#substituting-in-script-blocks

This is considered to be a bad practice[1]. Also breaks trusted
artifacts due to the script being reformatted and an extra space added
around the parentheses.

[1] https://tekton.dev/docs/pipelines/tasks/#substituting-in-script-blocks
@lcarva
Copy link
Contributor

lcarva commented Jun 12, 2024

Not sure about this. I believe the bad practice being called out is regarding using values from $(params.X) since a user could set that to whatever. $(results.X.path) is managed by Tekton. A Task user can't really exploit it by just running the Task. There are a lot of instances across the Tasks in this repo that use this pattern. I'd be cautious about making a fix at this scope if it's not really necessary.

Obviously, we need to fix the issue with the git-clone-oci-ta.yaml Task.

@simonbaird
Copy link
Contributor

simonbaird commented Jun 12, 2024

Griping in general, not about this PR: Why tf does the bash formatter want to add that space?

@simonbaird
Copy link
Contributor

I think we could merge this as a pragmatic solution then consider the other options later.

Do we think there are other tasks impacted apart from git-clone?

@lcarva
Copy link
Contributor

lcarva commented Jun 13, 2024

#1074 supersedes this. The fix was to tweak the tash tool to not add the space.

@openshift-merge-robot
Copy link

PR needs rebase.

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-sigs/prow repository.

@zregvart zregvart closed this Jun 14, 2024
@zregvart zregvart deleted the pr/dont-use-substitution-in-script branch June 14, 2024 06:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants