Refactor the usage of MustCompile and simplify function signatures #4927

chuangw6 · 2022-06-01T21:10:33Z

Changes

Prior to this commit:

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.
function extractExpressionFromString and extractVariablesFromString
return too many values i.e. extracted value (slice/string), present (bool)
and errorMessage (string)

In this fix:

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.
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 · 2022-06-01T21:15:10Z

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 · 2022-06-01T21:15:23Z

/assign @lbernick

tekton-robot · 2022-06-01T22:43:48Z

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

pkg/apis/pipeline/v1beta1/task_validation.go

tekton-robot · 2022-06-02T15:33:47Z

[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

pkg/apis/pipeline/v1beta1/task_validation.go

pkg/substitution/substitution_test.go

pkg/substitution/substitution.go

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](https://pkg.go.dev/regexp#MustCompile)). 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.

tekton-robot · 2022-06-02T17:48: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/apis/pipeline/v1beta1/task_validation.go	97.7%	97.7%	-0.0
pkg/substitution/substitution.go	57.9%	52.9%	-5.0

chuangw6 · 2022-06-02T18:09:26Z

/test tekton-pipeline-unit-tests

chuangw6 · 2022-06-02T18:53:07Z

/test pull-tekton-pipeline-integration-tests

chuangw6 · 2022-06-02T22:07:20Z

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

chuangw6 · 2022-06-03T01:46:36Z

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

lbernick · 2022-06-06T13:29:37Z

pkg/substitution/substitution.go

 	return nil
 }

 // Extract a the first full string expressions found (e.g "$(input.params.foo)"). Return


nit: typo in this docstring (extra "a")

pkg/substitution/substitution.go

lbernick · 2022-06-06T13:33:07Z

pkg/substitution/substitution.go

+	vs, err := extractVariablesFromString(value, prefix)
+	if err != nil {
+		return &apis.FieldError{
+			Message: fmt.Sprintf("fail to extract variables from string: %v", err),


I think you can just use the error message as is (here and at the other call sites)

lbernick · 2022-06-06T13:35:04Z

pkg/substitution/substitution.go

-					}
+	}
+
+	firstMatch, _ := extractExpressionFromString(value, prefix)


probably best to handle the error being returned by this function

pritidesai · 2022-06-07T04:46:19Z

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 · 2022-06-07T16:59:34Z

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 · 2022-06-07T19:25:07Z

/retest

pritidesai · 2022-06-08T17:39:39Z

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 · 2022-06-08T18:52:58Z

thanks @pritidesai, sgtm!

tekton-robot · 2022-06-14T04:24:03Z

@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 · 2022-06-14T15:00:10Z

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

lbernick · 2022-06-15T14:18:43Z

@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 · 2022-06-20T10:45:04Z

@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 · 2022-09-18T11:02:31Z

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 · 2022-09-27T19:22:18Z

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

chuangw6 · 2022-09-27T20:02:50Z

@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.

tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jun 1, 2022

tekton-robot requested review from jerop and lbernick June 1, 2022 21:10

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 1, 2022

chuangw6 mentioned this pull request Jun 1, 2022

[TEP-0075] Validate against using the whole object in task steps #4861

Merged

5 tasks

tekton-robot assigned lbernick Jun 1, 2022

chuangw6 force-pushed the change-mustcompile branch from fdc8963 to 0a7871e Compare June 1, 2022 22:41

vdemeester reviewed Jun 2, 2022

View reviewed changes

pkg/apis/pipeline/v1beta1/task_validation.go Show resolved Hide resolved

vdemeester approved these changes Jun 2, 2022

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 2, 2022

lbernick reviewed Jun 2, 2022

View reviewed changes

pkg/apis/pipeline/v1beta1/task_validation.go Show resolved Hide resolved

pkg/substitution/substitution_test.go Outdated Show resolved Hide resolved

pkg/substitution/substitution.go Show resolved Hide resolved

chuangw6 force-pushed the change-mustcompile branch from 0a7871e to 2c7ad87 Compare June 2, 2022 17:44

chuangw6 requested a review from lbernick June 2, 2022 18:29

lbernick reviewed Jun 6, 2022

View reviewed changes

pritidesai mentioned this pull request Jun 7, 2022

TEP-0090: Split up ResolvePipelineRunTask #4943

Merged

4 tasks

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 14, 2022

tekton-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 18, 2022

chuangw6 marked this pull request as draft September 27, 2022 20:00

tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 27, 2022

chuangw6 closed this Sep 27, 2022

Refactor the usage of MustCompile and simplify function signatures #4927

Refactor the usage of MustCompile and simplify function signatures #4927

Uh oh!

Conversation

chuangw6 commented Jun 1, 2022

Changes

Submitter Checklist

Release Notes

Uh oh!

tekton-robot commented Jun 1, 2022

Uh oh!

chuangw6 commented Jun 1, 2022

Uh oh!

tekton-robot commented Jun 1, 2022

Uh oh!

Uh oh!

tekton-robot commented Jun 2, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tekton-robot commented Jun 2, 2022

Uh oh!

chuangw6 commented Jun 2, 2022

Uh oh!

chuangw6 commented Jun 2, 2022

Uh oh!

chuangw6 commented Jun 2, 2022

Uh oh!

chuangw6 commented Jun 3, 2022

Uh oh!

lbernick Jun 6, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lbernick Jun 6, 2022

Choose a reason for hiding this comment

Uh oh!

lbernick Jun 6, 2022

Choose a reason for hiding this comment

Uh oh!

pritidesai commented Jun 7, 2022

Uh oh!

lbernick commented Jun 7, 2022

Uh oh!

abayer commented Jun 7, 2022

Uh oh!

pritidesai commented Jun 8, 2022

Uh oh!

lbernick commented Jun 8, 2022

Uh oh!

tekton-robot commented Jun 14, 2022

Uh oh!

chuangw6 commented Jun 14, 2022

Uh oh!

lbernick commented Jun 15, 2022

Uh oh!

vdemeester commented Jun 20, 2022

Uh oh!

tekton-robot commented Sep 18, 2022

Uh oh!

lbernick commented Sep 27, 2022

Uh oh!

chuangw6 commented Sep 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants