-
Notifications
You must be signed in to change notification settings - Fork 220
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
TEP-Trim Trailing Whitespace of Tekton Result. #223
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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This looks good to me. |
… a [bug](kubeflow/kfp-tekton#273) of kubeflow/kfp-tekton, aims to strip the EOF new line and all the unwanted trailing whitespace.
Hi @popcor255 , |
Thanks for writing this up @XinruZhang ! I am actually against adding this feature, because I feel we should try to keep our API as simple as possible (e.g. see design principles on simplicity) One alternative that I'd like to see represented in the alternatives section is not implementing this - i.e. what does it look like to have to trim the whitespace yourself? To go from the example in the TEP: steps:
- name: main
image: alpine:3.12
script: |
echo "$(params.project_name)" > "$(results.project.path)" To a version that will not contain the trailing whitespace is a matter of using a different command, e.g. printf: steps:
- name: main
image: alpine:3.12
script: |
printf "$(params.project_name)" > "$(results.project.path)" To see this in action on the command line: # echo will add a newline
~ echo "foo" > bar
~ cat bar
foo
# printf will not
printf "foo" > bar
~ cat bar
foo% Basically I think we should only add Tekton level features when it is impossible or extremely cumbersome to have the functionality without Tekton supporting it, and that doesn't seem to be the case here. Plus this opens the floodgates (as mentioned in the TEP) of folks asking for other trimming support, or other features (capitalization, replacement). I totally understand that this gotcha is frustrating and have run into it myself a few times - however adding a bool that does the trimming will not save you from this gotcha as you will still need to be aware of when to set it, so I'm not convinced that this feature will is necessary. |
But you can probably change my mind pretty easily here! I just need to understand what it's important for this to be a feature tekton pipelines provides and it's not enough to ask folks to do the trimming when they need it within their steps (or in the echo/printf case, not so much "trim" as "not output a newline at all when not needed") |
@bobcatfish Thanks for your question! Your concern makes a lot sense. Here is the reason for adding a new field. Assigning a value to a result is usually by redirecting the output stream of a command to the result file. There are lots of commands whose default output stream is As for opening the floodgates of asking other related features support, I guess there would be few users requring replacement and capitalization features, and If they ask for, we can appraise the necessity then. |
Thanks for the extra info @XinruZhang ! Before going ahead, can we gather some examples of scenarios where we want to capture the stdout of a command and provide it straight to a result and an extra newline causes a problem?
Thanks for these examples! In these cases, I feel like it would be reasonable to add an additional step that does whatever formatting is needed. I'll show you a few examples.
So basically TL;DR I think it's reasonable to have Task authors add extra steps to do whatever formatting they need vs. doing this for them. |
Hi @bobcatfish , Thanks so much for your comment, It’s very important to make this clear. In fact, I'm quite shifty about this problem. After doing some more research, I'm currently inclined to add a new boolean field to tackle the unwanted line break other than leaving it to authors. I believe keeping Tekton simple is very important. My concern is the current version is somehow not very user-friendly. Here are some detailed reasons:
I'm inclined to believe that adding a new field is a better way to tackle this problem based on the analysis I did. In the meantime, I'm not very firmly because I'm new to Tekton, so I guess whether to carry forward this TEP is up to you~ |
Thanks for the detailed explanation @XinruZhang and for finding those examples! I think the points you make make sense, however I think you could also make a similar statement about other mutations you might want to do on a result, e.g. filtering, extracting a value from a json object, etc. In all of these cases a user would need to understand what the output is of the command they are running vs. what value they want in the result. It's interesting that you highlighted "user-friendliness" as the main motivation here - two thoughts:
To me (2) means not that we want to make something that is intentionally difficult to use, but that adding features which make some specific action a bit easier (specifically: you don't need to add a line to a script or add a step to remove the newline) is not a priority for the project. I think this is especially true given that we are gearing up toward a v1 release (would be interested in @vdemeester's thoughts on this) - I could see us revisiting this post v1, i.e.: now that we are confident in our v1 release, do we want to expand into additional functionality such as mutating results? Or does it perhaps make sense to introduce some new component / feature to the API which can express these sorts of desired mutations?
To me what the simple principle means, and how I've been applying it so far to the project is: if we can avoid adding a feature, let's avoid it, until we absolutely have to. |
@bobcatfish That's excellent! Thanks for your explanations~ I totally understand now. The function this TEP aims to provide can be tackled by users themselves, there's no need to change Tekton just for refining Task results in the current stage. I guess this PR can be closed then. BTW, I closed this directly by mistake, what do you think @imjasonh @sbwsg @popcor255 |
This TEP is for issue #3146 originated from a bug of kubeflow/kfp-tekton, aims to strip the EOF new line and all the unwanted trailing whitespace.