Refactor the usage of MustCompile and simplify function signatures

[chuangw6]

Changes

Prior to this commit:

1. MustCompile sometimes is used inside a function though it is meant for initializing global variables holding compiled regular expressions according to this (docs). Using it in a function could possibly panic.
2. function extractExpressionFromString and extractVariablesFromString return too many values i.e. extracted value (slice/string), present (bool) and errorMessage (string)

In this fix:

1. regex compiled from from MustCompile is changed to be global, or MustCompile is replaced with Compile so that we can return error rather than causing potential panic.
2. the two functions signatures are simplified to just return extracted value and error.

Submitter Checklist

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

• [ ] Docs included if any changes are user facing
• [ ] Tests included if any functionality added or changed
• [ ] Follows the commit message standard
• [ ] Meets the Tekton contributor standards (including functionality, content, code)
• [ ] Release notes block below has been filled in (if there are no user facing changes, use release note "NONE")

Release Notes

[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/apis/pipeline/v1beta1/task_validation.go	97.7%	97.7%	-0.0
pkg/substitution/substitution.go	57.9%	52.9%	-5.0

[chuangw6]

/assign @lbernick

[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/apis/pipeline/v1beta1/task_validation.go	97.7%	97.7%	-0.0
pkg/substitution/substitution.go	57.9%	52.9%	-5.0

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester

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~~ [vdemeester]

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

[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/apis/pipeline/v1beta1/task_validation.go	97.7%	97.7%	-0.0
pkg/substitution/substitution.go	57.9%	52.9%	-5.0

[chuangw6]

/test tekton-pipeline-unit-tests

[chuangw6]

/test pull-tekton-pipeline-integration-tests

[chuangw6]

/test pull-tekton-pipeline-alpha-integration-tests

[chuangw6]

/test pull-tekton-pipeline-alpha-integration-tests

[pritidesai]

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/apis/pipeline/v1beta1/task_validation.go 97.7% 97.7% -0.0 pkg/substitution/substitution.go 57.9% 52.9% -5.0

@chuangw6 thank you for this 🙏

I recommend adding more unit test. Impacting 5% coverage during the refactor is not ideal specially with such low coverage to begin with 🔅 .

Please try to make sure we at least maintain the code coverage as is in a refactoring PRs. substitution has been one of the packages with very low coverage 😞.

@vdemeester @lbernick thoughts?

[lbernick]

I recommend adding more unit test. Impacting 5% coverage during the refactor is not ideal specially with such low coverage to begin with 🔅 .

Please try to make sure we at least maintain the code coverage as is in a refactoring PRs. substitution has been one of the packages with very low coverage 😞.

Definitely agree more testing would be nice here for these helpers! Not sure how much to read into the coverage tool output or how much testing of this package is appropriate to tackle for a refactor-- will leave that up to Chuang

[abayer]

/retest

[pritidesai]

I recommend adding more unit test. Impacting 5% coverage during the refactor is not ideal specially with such low coverage to begin with 🔅 . Please try to make sure we at least maintain the code coverage as is in a refactoring PRs. substitution has been one of the packages with very low coverage 😞.

Definitely agree more testing would be nice here for these helpers! Not sure how much to read into the coverage tool output or how much testing of this package is appropriate to tackle for a refactor-- will leave that up to Chuang

Thanks @lbernick!

Please look for the no coverage represented by the code in red 🟥

Thanks @chuangw6 for your understanding 👍 Appreciate all the simplifications here, the substitution package is much more clean now 👏 Please add more coverage for the changes you are introducing.

An alternative could be to introduce the unit tests first in a separate PR to bump the existing coverage.

[lbernick]

thanks @pritidesai, sgtm!

[tekton-robot]

@chuangw6: 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/test-infra repository.

[chuangw6]

Thanks @pritidesai @lbernick . sgtm! I'll add that!

[lbernick]

@pritidesai in light of our discussion yesterday at the pipelines WG around not testing non-exported functions, what do you think is the best way forward here?

[vdemeester]

@pritidesai in light of our discussion yesterday at the pipelines WG around not testing non-exported functions, what do you think is the best way forward here?

Ideally even if we are not testing non exported functions, we should cover / aim to cover them (through exported ones). If we can’t, this usually mean the unexported function covers more cases that what is exported (and thus is more complex that it needs to be 🤭).

[tekton-robot]

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale with a justification. Stale issues rot after an additional 30d of inactivity and eventually close. If this issue is safe to close now please do so with /close with a justification. If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle stale

Send feedback to tektoncd/plumbing.

[lbernick]

@chuangw6 are you still working on this? would you mind rebasing or closing?

[chuangw6]

@chuangw6 are you still working on this? would you mind rebasing or closing?

Sorry I saw this as non-urgent and switched to other stuff before. Happy to get this cleaned up and move it forward.