-
Notifications
You must be signed in to change notification settings - Fork 123
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
Support the comma as default string param separator #921
Conversation
@Tomcli could you please review this PR? Thanks, this is a valid case on our customer side. |
The ut failure was not caused by my code, it was caused by the cache store. |
@wzhanw Although the caching code is returning error logs due to not running on Kubernetes, that didn't cause the unit test to fail. The only failed unit test is missing this error message. kfp-tekton/tekton-catalog/pipeline-loops/pkg/reconciler/pipelinelooprun/pipelinelooprun_test.go Lines 2074 to 2080 in 570c7de
|
tekton-catalog/pipeline-loops/pkg/reconciler/pipelinelooprun/pipelinelooprun.go
Show resolved
Hide resolved
@wzhanw Thanks for the PR. Your use case make sense. Once the unit tests are fixed, I can merge this PR |
@Tomcli thanks, let me fix the failure in the unit test. |
@Tomcli please review again, I fixed the unit test failure. |
thanks @wzhanw |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Tomcli, wzhanw 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 |
Which issue is resolved by this Pull Request:
Resolves #
We have a use case like that:
The iteration parameter string value is
1,2,3,4,5
, but the separator is not set.User wants the default separator should be a comma
(,)
Description of your changes:
Try the default separator
,
in the last wheninterationParam
string is not empty and contains the comma.Environment tested:
Unit test passed.
python --version
):tkn version
):kubectl version
):/etc/os-release
):Checklist: