fix: stepresult intepolations does not accept multiple matches #7830

ericzzzzzzz · 2024-04-03T18:55:54Z

Changes

This pr enables users to use multiple step results for interpolation in step command and environment variables, for examples, users can do something like "$(steps.foo.results.res.first)-$(steps.foo.results.res.second)" set env variables for step.

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps
Has Tests included if any functionality added or changed
pre-commit Passed
Follows the commit message standard
Meets the Tekton contributor standards (including functionality, content, code)
Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings). See some examples of good release notes.
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

 fix: cannot use multiple step results at the same time for interpolation.

tekton-robot · 2024-04-03T18:55:56Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

ericzzzzzzz · 2024-04-03T18:56:04Z

/kind fix

tekton-robot · 2024-04-03T18:56:06Z

@ericzzzzzzz: The label(s) kind/fix cannot be applied, because the repository doesn't have them.

In response to this:

/kind fix

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.

ericzzzzzzz · 2024-04-03T18:58:48Z

convert pr status to review for testing ci, but this is not really ready for review

ericzzzzzzz · 2024-04-03T18:58:57Z

/hold

ericzzzzzzz · 2024-04-03T18:59:23Z

/kind bug

ericzzzzzzz · 2024-04-03T18:59:36Z

/kind bug

tekton-robot · 2024-04-03T19:05:51Z

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

File	Old Coverage	New Coverage	Delta
pkg/entrypoint/entrypointer.go	89.0%	89.2%	0.3

tekton-robot · 2024-04-03T19:10:09Z

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/entrypoint/entrypointer.go	89.0%	89.2%	0.3

ericzzzzzzz · 2024-04-03T19:41:40Z

/hold cancel

ericzzzzzzz · 2024-04-03T19:41:58Z

This is ready for review now ! thanks

chitrangpatel

Thanks @ericzzzzzzz !!

chitrangpatel · 2024-04-05T14:50:56Z

/cherry-pick release-v0.56.x

tekton-robot · 2024-04-05T14:50:59Z

@chitrangpatel: once the present PR merges, I will cherry-pick it on top of release-v0.56.x in a new PR and assign it to you.

In response to this:

/cherry-pick release-v0.56.x

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.

chitrangpatel · 2024-04-05T14:51:23Z

@vdemeester, @JeromeJu PTAL 🙏

It will be nice to get this in before the bugfix release.

tekton-robot · 2024-04-11T01:20:25Z

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/entrypoint/entrypointer.go	89.0%	89.2%	0.3

tekton-robot · 2024-04-11T01:20:53Z

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

File	Old Coverage	New Coverage	Delta
pkg/entrypoint/entrypointer.go	89.0%	89.2%	0.3

tekton-robot · 2024-04-11T02:20:08Z

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/entrypoint/entrypointer.go	89.0%	89.2%	0.3

tekton-robot · 2024-04-11T02:28:51Z

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

File	Old Coverage	New Coverage	Delta
pkg/entrypoint/entrypointer.go	89.0%	89.2%	0.3

JeromeJu

Thanks @ericzzzzzzz generally lgtm just some nits.

JeromeJu · 2024-04-11T15:20:08Z

pkg/entrypoint/entrypointer.go

 // replaceCommandAndArgs performs replacements for step results in e.Command
 func replaceCommandAndArgs(command []string, stepDir string) ([]string, error) {
-	newCommand := []string{}
+	var newCommand []string


NIT:

Suggested change

var newCommand []string

var newCommands []string

I think command is supposed to be singular, although it's a slice of string, because it is a command which does one thing and invoking a sub-command from a program requires its parent command, like ["gcloud", "run", "deploy"], we normally consider this as one command. Commands seems to indicate there are multiple commands to do many things. K8s spec for container also uses singular, although it's an a list of string.

JeromeJu · 2024-04-11T15:20:48Z

pkg/entrypoint/entrypointer.go

 func replaceCommandAndArgs(command []string, stepDir string) ([]string, error) {
-	newCommand := []string{}
+	var newCommand []string
+flag:


dumb question: is this the best practice to have this here?

Wondering if we change L439 to newC = replaceWithArray... can we move off this flag? - but will definitely leave for your decision on the implementation.

I changed this a little to make it more readable

JeromeJu · 2024-04-11T15:23:43Z

pkg/entrypoint/entrypointer.go

+		for _, m := range matches {
+			replaceWithString, replaceWithArray, err := findReplacement(stepDir, m[0])
+			if err != nil {
+				return []string{}, err


Shall we consider wrapping the error here with the specific command arg that it fails on?

tekton-robot · 2024-04-11T15:26:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chitrangpatel, JeromeJu

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~~ [JeromeJu,chitrangpatel]

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

tekton-robot · 2024-04-11T16:40:31Z

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/entrypoint/entrypointer.go	86.7%	86.8%	0.1

tekton-robot · 2024-04-11T16:43:24Z

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

File	Old Coverage	New Coverage	Delta
pkg/entrypoint/entrypointer.go	86.7%	86.8%	0.1

vdemeester

/lgtm

tekton-robot · 2024-04-12T09:44:14Z

@chitrangpatel: new pull request created: #7875

In response to this:

/cherry-pick release-v0.56.x

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.

tekton-robot added release-note-none Denotes a PR that doesnt merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Apr 3, 2024

tekton-robot requested review from afrittoli and dibyom April 3, 2024 18:55

tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 3, 2024

ericzzzzzzz marked this pull request as ready for review April 3, 2024 18:58

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 3, 2024

tekton-robot requested a review from vdemeester April 3, 2024 18:58

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 3, 2024

tekton-robot added the kind/bug Categorizes issue or PR as related to a bug. label Apr 3, 2024

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesnt merit a release note. labels Apr 3, 2024

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 3, 2024

chitrangpatel approved these changes Apr 3, 2024

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 3, 2024

chitrangpatel added this to the Pipelines v0.59 milestone Apr 4, 2024

ericzzzzzzz force-pushed the step-results-intepolations branch from adefe3f to 1855c8d Compare April 11, 2024 01:10

ericzzzzzzz force-pushed the step-results-intepolations branch from 1855c8d to 197e565 Compare April 11, 2024 02:11

JeromeJu approved these changes Apr 11, 2024

View reviewed changes

fix: stepresult-intepolations-does-not-accept-multiple-matches

5575605

ericzzzzzzz force-pushed the step-results-intepolations branch from 197e565 to 5575605 Compare April 11, 2024 16:33

vdemeester reviewed Apr 12, 2024

View reviewed changes

tekton-robot assigned vdemeester Apr 12, 2024

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 12, 2024

tekton-robot merged commit 969a98f into tektoncd:main Apr 12, 2024

tekton-robot mentioned this pull request Apr 12, 2024

[release-v0.56.x] fix: stepresult intepolations does not accept multiple matches #7875

Merged

fix: stepresult intepolations does not accept multiple matches #7830

fix: stepresult intepolations does not accept multiple matches #7830

Uh oh!

Conversation

ericzzzzzzz commented Apr 3, 2024 • edited by chitrangpatel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Submitter Checklist

Release Notes

Uh oh!

tekton-robot commented Apr 3, 2024

Uh oh!

ericzzzzzzz commented Apr 3, 2024

Uh oh!

tekton-robot commented Apr 3, 2024

Uh oh!

ericzzzzzzz commented Apr 3, 2024

Uh oh!

ericzzzzzzz commented Apr 3, 2024

Uh oh!

ericzzzzzzz commented Apr 3, 2024

Uh oh!

ericzzzzzzz commented Apr 3, 2024

Uh oh!

tekton-robot commented Apr 3, 2024

Uh oh!

tekton-robot commented Apr 3, 2024

Uh oh!

ericzzzzzzz commented Apr 3, 2024

Uh oh!

ericzzzzzzz commented Apr 3, 2024

Uh oh!

chitrangpatel left a comment

Choose a reason for hiding this comment

Uh oh!

chitrangpatel commented Apr 5, 2024

Uh oh!

tekton-robot commented Apr 5, 2024

Uh oh!

chitrangpatel commented Apr 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tekton-robot commented Apr 11, 2024

Uh oh!

tekton-robot commented Apr 11, 2024

Uh oh!

tekton-robot commented Apr 11, 2024

Uh oh!

tekton-robot commented Apr 11, 2024

Uh oh!

JeromeJu left a comment

Choose a reason for hiding this comment

Uh oh!

JeromeJu Apr 11, 2024

Choose a reason for hiding this comment

Uh oh!

ericzzzzzzz Apr 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JeromeJu Apr 11, 2024

Choose a reason for hiding this comment

Uh oh!

JeromeJu Apr 11, 2024

Choose a reason for hiding this comment

Uh oh!

ericzzzzzzz Apr 11, 2024

Choose a reason for hiding this comment

Uh oh!

JeromeJu Apr 11, 2024

Choose a reason for hiding this comment

Uh oh!

tekton-robot commented Apr 11, 2024

Uh oh!

tekton-robot commented Apr 11, 2024

Uh oh!

tekton-robot commented Apr 11, 2024

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

ericzzzzzzz commented Apr 3, 2024 •

edited by chitrangpatel

Loading

chitrangpatel commented Apr 5, 2024 •

edited

Loading

ericzzzzzzz Apr 11, 2024 •

edited

Loading