-
Notifications
You must be signed in to change notification settings - Fork 124
chore: Refactor GenerateTaskRun with improved modularity and naming consistency #1966
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
base: main
Are you sure you want to change the base?
Conversation
0fa0d78
to
53114d8
Compare
53114d8
to
bffbea6
Compare
/retest |
c3e3921
to
1c25d88
Compare
/lgtm |
@ayushsatyam146 PR needs rebase, looks like there is a merge conflict with one of the test files. |
1c25d88
to
ff524ce
Compare
New changes are detected. LGTM label has been removed. |
Hi @adambkaplan I resolved the merge conflict here, PTAL thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First off, I want to commend you @ayushsatyam146 for this submisssion. The refactored form of taskrun.go
is much clearer to read and will make it easier to add future contributions. I especially like that the features which apply to the TaskRun as a whole (node selector, annotations, etc.) are separated from features that apply to specific build steps (obtaining source code, processing the output image, etc.).
The refactor in taskrun_test.go
likewise adds coverage that was missing and/or makes it clearer which behaviors are being verified at the "unit" level. In particular, the following test cases are improved:
- Handling of parameter types + defaults
- Handling of volumes (definition, overrides)
- Checks that verify the image processing step is included
That said, it appears there are a few checks in the original code that are missing. Given that we want to release v0.17.0 soon, this PR as is presents a risk due to removed test coverage. Based on my analysis (with the help of Cursor), a few checks were removed:
- Verify the arguments passed to the
image-processing
step - Verify the existence of the source clone step and its arguments
- Verify the compute resources are correctly specified in the TaskRun
I tried to identify these specific missing tests/checks in my comments.
/approve Green-lighting this as a "feature" to be added. /hold The size of change, and impact on test coverage, presents a risk to the release. My vote is to hold this PR until after the release, at which point we can merge it early in the v0.18.0 lifecycle. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adambkaplan 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 |
/hold cancel v0.17.0 is released! |
ff524ce
to
fc6371d
Compare
The existing TaskRun generation logic was concentrated in a single monolithic GenerateTaskRun function, making it difficult to maintain, test, and debug individual configuration aspects. This created tight coupling and poor separation of concerns. Decomposed the monolithic GenerateTaskRun into a comprehensive function. orchestrated by smaller, single-responsibility helper functions. Each configuration aspect now has its own dedicated applyXFeature* function. This approach enables better error isolation, easier unit testing of individual components, and clearer code organization. Enhanced error messages now provide specific context about which BuildRun and strategy caused failures, improving debugging experience for operators. Key architectural changes: - Made GenerateTaskRun as the main entry point and removed the GenerateTaskSpec - Extracted applyNodeSelectors, applyTolerations, applyScheduler functions - Renamed addBuildStrategySteps → applyBuildStrategySteps for consistency - Renamed addStrategyParametersToTaskSpec → applyStrategyParametersToTaskSpec - Introduced initializeTaskRun, buildTaskSpec, and createBaseTaskSpec helpers - Improved error messages with BuildRun and strategy context Signed-off-by: Ayush Satyam <[email protected]>
fc6371d
to
e998096
Compare
Changes
The existing TaskRun generation logic was concentrated in a single monolithic
GenerateTaskRun function, making it difficult to maintain, test, and debug
individual configuration aspects. This created tight coupling and poor separation
of concerns.
Decomposed the monolithic GenerateTaskRun into a comprehensive function.
orchestrated by smaller, single-responsibility helper functions. Each
configuration aspect now has its own dedicated applyXFeature* function.
This approach enables better error isolation, easier unit testing of individual
components, and clearer code organization. Enhanced error messages now provide
specific context about which BuildRun and strategy caused failures, improving
debugging experience for operators.
Key architectural changes:
Submitter Checklist
See the contributor guide
for details on coding conventions, github and prow interactions, and the code review process.
Release Notes