-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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 extra lines in new-app output #13540
Conversation
@openshift/devex @smarterclayton ptal |
pkg/generate/app/cmd/template.go
Outdated
s := fmt.Sprintf("%s%s\n", tab, lval) | ||
if len(strings.TrimSpace(s)) > 0 { | ||
fmt.Fprintf(out, fmt.Sprintf("%s%s\n", tab, lval)) | ||
} |
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.
i don't know that we care that much but this is going to strip out newlines in a string like "foo\n\nbar\n" (ie there's an intentional extra newline in the description and this will strip it).
might be better to just strip all trailing newlines from the string via a regex before doing the split.
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.
Seems reasonable to me @bparees
Researching how the existing implementation got where it was, @juanvallejo added the formatString
method in #12724 as part of addressing https://bugzilla.redhat.com/show_bug.cgi?id=1370104 for another look and feel complaint against output from the CLI.
Far as I can tell from the comments in those 2 items, there was no explicit intent to strip out newlines from things like "foo\n\nbar\n" ... does that sound right to you @juanvallejo ?
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.
Far as I can tell from the comments in those 2 items, there was no explicit intent to strip out newlines from things like "foo\n\nbar\n" ... does that sound right to you @juanvallejo ?
Correct, I believe the original approach for formatString
only focused on making sure that all new lines in a given string had the same level of indentation
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.
thanks @juanvallejo
@bparees - updated change ptal
lgtm [merge] |
Hit flake #12007 and flake #12060 and flake kubernetes/kubernetes#38573 Those were the three failures during the conformance install run; everything else clean @bparees please re-post merge comment at your convenience |
[merge]
…On Mon, Mar 27, 2017 at 2:20 PM, Gabe Montero ***@***.***> wrote:
Hit flake #12007 <#12007> and
flake #12060 <#12060> and flake
kubernetes/kubernetes#38573
<kubernetes/kubernetes#38573>
Those were the three failures during the conformance install run;
everything else clean
@bparees <https://github.com/bparees> please re-post merge comment at
your convenience
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#13540 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEvl3od47lFICWN8a7TiWw5siubxQ0Twks5rp_3TgaJpZM4Mow77>
.
--
Ben Parees | OpenShift
|
Evaluated for origin merge up to 334cca2 |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/208/) (Base Commit: 345cb97) (Image: devenv-rhel7_6104) |
Fixes #13492
Output now: