chore(backend): Consolidate v2 placeholder resolution into shared utility. Fixes #12949#13027
chore(backend): Consolidate v2 placeholder resolution into shared utility. Fixes #12949#13027khushiiagrawal wants to merge 6 commits intokubeflow:masterfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @khushiiagrawal. Thanks for your PR. I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
|
@droctothorpe @hbelmiro PTAL. thanks. |
There was a problem hiding this comment.
Pull request overview
This PR centralizes v2 input-parameter placeholder substitution and structpb.Value → string conversion into a new shared backend/src/v2/placeholder package, then updates both the driver and launcher to use that canonical implementation (including proper handling of null values).
Changes:
- Added
backend/src/v2/placeholderwith shared utilities:PbValueToString,ResolveInputParameterPlaceholders, andInputParameterRe. - Refactored
backend/src/v2/driver/util.goto call the shared placeholder resolver instead of maintaining a duplicate implementation. - Refactored
backend/src/v2/component/launcher_v2.goto usePbValueToString, fixing priornullhandling for input parameters.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| backend/src/v2/placeholder/placeholder.go | New shared placeholder/type-stringification utility used by both driver and launcher. |
| backend/src/v2/placeholder/placeholder_test.go | Unit tests covering value stringification and placeholder substitution scenarios. |
| backend/src/v2/driver/util.go | Removes duplicated placeholder logic and routes resolution through the shared package. |
| backend/src/v2/component/launcher_v2.go | Replaces inline structpb.Value kind switching with PbValueToString (incl. null). |
| // InputParameterRe matches the complete {{$.inputs.parameters['name']}} | ||
| // placeholder including the surrounding double braces. | ||
| var InputParameterRe = regexp.MustCompile(`\{\{\$\.inputs\.parameters\['(.+?)']}}`) |
…ers and update related functions Signed-off-by: khushiiagrawal <khushisaritaagrawal@gmail.com>
Signed-off-by: khushiiagrawal <khushisaritaagrawal@gmail.com>
3db5039 to
0e79421
Compare
… normalization utility Signed-off-by: khushiiagrawal <khushisaritaagrawal@gmail.com>
Signed-off-by: Khushi Agrawal <149886195+khushiiagrawal@users.noreply.github.com>
|
@khushiiagrawal can you please rebase? |
Description of your changes:
This PR consolidates duplicate placeholder resolution and type stringification logic that existed in both the driver and launcher into a shared utility package, addressing technical debt from PR #12598.
Fixes #12949
What changed:
Created new
backend/src/v2/placeholder/package with:PbValueToString()- canonicalstructpb.Valueto string conversionResolveInputParameterPlaceholders()- shared input parameter placeholder resolutionInputParameterRe- exported regex pattern for placeholder matchingUpdated
backend/src/v2/driver/util.go:pbValueToString,resolveSinglePlaceholder,resolveInputParameterPlaceholdersfunctionsUpdated
backend/src/v2/component/launcher_v2.go:Why:
Checklist: