Skip to content
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

fix 954, trim whitespace and newline char for range input #961

Merged
merged 2 commits into from
May 27, 2022

Conversation

wzhanw
Copy link
Contributor

@wzhanw wzhanw commented May 27, 2022

Which issue is resolved by this Pull Request:
Resolves # #954

Description of your changes:

  1. Trim space and newline char for range input and validate it
  2. Allow whitespace as iteration string.

Environment tested:

  • Python Version (use python --version):
  • Tekton Version (use tkn version):
  • Kubernetes Version (use kubectl version):
  • OS (e.g. from /etc/os-release):

Checklist:

@wzhanw
Copy link
Contributor Author

wzhanw commented May 27, 2022

@Tomcli @ScrapCodes could you review and approve this? we required this fix asap.

for _, p := range run.Spec.Params {
if p.Name == "from" {
from, _ = strconv.Atoi(p.Value.StringVal)
fromProvided = true
from, err = getIntegerParamValue(p)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In practice, I have not seen a problem due to "\n" terminated strings passed as a value. My understanding was these are already taken care of by the parsers.

Can you provide an example ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ScrapCodes , we have an example, the value of the range is from an upper task result, e.g: an output of a bash script task. The result is 3\n I see.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are from and to parameters of a loop, do you think they are generated as a result of running a bash script?

Copy link
Contributor Author

@wzhanw wzhanw May 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, we have a pipeline, the upper node is a bash script task and its output will be the input of the pipeline loop run.

@ScrapCodes
Copy link
Contributor

/lgtm

@ScrapCodes
Copy link
Contributor

/approve

@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ScrapCodes, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-prow google-oss-prow bot merged commit 6b3ef55 into kubeflow:master May 27, 2022
rimolive pushed a commit to opendatahub-io/data-science-pipelines-tekton that referenced this pull request May 27, 2022
* fix 954, trim whitespace and newline char for range input

* address review comments
@wzhanw wzhanw mentioned this pull request May 31, 2022
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants