[refactor] Remove ImplicitParamEnabled context. #4528
Conversation
This change does not introduce any different behavior - it refactors the code to remove the use of the ImplicitParamEnabled context in favor of direct function calls.
|
/cc @jerop |
|
The following is the coverage report on the affected files.
|
There was a problem hiding this comment.
i have a couple of comments about the tests - in particular im not clear on what the intention is of the TestImplicitParams_Pipeline test when that defaulting logic shouldn't include the implicit params logic (maybe it's just a naming or commenting thing to clear that up, but im thinking more focused tests might help here)
but in the meantime this has been open for a while and think the codebase is in a better place with this PR in (i.e. more explicit than it was when we were using flags in the context variable to change function behavior) so id like to see this get in even without those tweaks (if needed at all)
/approve
/lgtm
| @@ -314,160 +297,53 @@ func TestImplicitParams_Pipeline(t *testing.T) { | |||
|
|
|||
There was a problem hiding this comment.
Im a bit confused about why the Pipeline spec has an implicit params test - it seems like in the updated version, the implicit params logic is only called from the pipelinerun and taskrun defaulting functions - i would think pipeline.SetDefaults wouldn't have any implicit param logic? (unless the purpose of this test is to ensure that the implicit params logic is not applied?)
one thing that might make this a bit clearer is to have test cases specifically for the new implicit params functions as well - the current test cases seem to be covering the existing defaulting logic as well so it's a bit hard to tell what is being caused by the implicit params logic and what is caused by the defaulting logic
| for _, tc := range []struct { | ||
| // Expect in and want to be the same type. | ||
| in apis.Defaultable | ||
| want interface{} |
There was a problem hiding this comment.
could you include a name to indicate the purpose of each test case? i think its one for a pipelinerun, one for a taskrun (and i think nothing more complicated than that?) but it wasnt easy to tell without reading through each test case
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bobcatfish 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:
Approvers can indicate their approval by writing |
Changes
This change does not introduce any different behavior - it refactors the
code to remove the use of the ImplicitParamEnabled context in favor of
direct function calls.
/kind cleanup
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
Release Notes