-
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
Adding template instructional message to new-app output #9806
Adding template instructional message to new-app output #9806
Conversation
ok, game on.... :) for those who need a refresher, here's what the new output looks like w/ this PR:
"lgtm" |
It does not stand out enough. On Tue, Jul 12, 2016 at 4:06 PM, Ben Parees [email protected]
|
I'd probably expect leading and trailing newlines between the message and On Tue, Jul 12, 2016 at 9:05 PM, Clayton Coleman [email protected]
|
let's put "MESSAGE:" in all caps. (kidding) On Tue, Jul 12, 2016 at 9:05 PM, Clayton Coleman [email protected]
Ben Parees | OpenShift |
I agree that "Message:" might not be the best label/prefix for the information. newline sounds like a good idea too. |
@smarterclayton @jupierce how about:
? |
Too heavy. I'd want it to look similar to how we display the
description of an image in new-app
|
image description like this?
Are you saying the message should just be a bullet that says something like "* This information: [insert message]"? (and then we'd make "With parameters" also a bullet?) putting it all together:
? |
No, that image has no description it looks like. The Ruby or Python
image should
|
So:
? (note i've retained the bullets for the parameters section, i think they are an improvement). I also removed the template name from the first line and used it in the header instead. |
Minor tweaks
|
i think the only tweak i see is the additional newline, works for me. For templates that have no message what will we display? just:
? |
or maybe
? |
Eh, if you have no message, just a newline between template and "with parameters" (no title, no description) |
Sorry, if you have no title or description. |
Actually, if we don't respect those for templates that may be something:
If nothing, just a newline |
I think we always have a title (it's the template metadata name). So if we're not going to display it as a "title" then we need to put it back in the first line ("deploying Simple for "simple") to at least make it clear somewhere what template we actually picked. |
The first line contains what the user entered (so the user knows that their input corresponded to something else). I was referring to the "display name" of the template. |
templates don't have "display names" one more try:
since metadata.name might be shown twice now, we can get fancier and say for: only print the metadata.name if we're not going to print it below. or we can just always print it twice. |
Why don't templates have display names? |
@smarterclayton it never came up. Can't you ask the same question about deploymentconfigs, buildconfigs, etc? |
(in other words if it's something we think we want, doesn't it belong on ObjectMeta?) |
The describers and the console should show display name for most things. |
It's an annotation we respect fairly consistently, and especially in new-app. |
well there's still no guarantee we have one, so replace all my "metadata.name" references above with "annotation.disiplayName || metadata.name" |
sure. |
With display-name, description, and message:
Graceful degradation after removing message and display-name:
And finally, removing description:
|
i'm happy with it, @smarterclayton may not like the redundancy of showing the template name twice. |
I don't. |
I think there is value in being able to glance at a consistent, highly prominent line and know what took place. This always benefits scripting/testing, but even with humans, your user (for reasons they will not understand) will otherwise sometimes see all the information they need in a concise message while at other times see a vague message and need to intuit the need to visually scan for a string floating above "---------". To that end, perhaps we can keep the --> line consistent/technical/parsable:
The optional
into:
This approach would ensure a well designed template (i.e. with a display-name) would not display any redundant information while being processed by new-app. Templates without display-names would have some redundancy, but it would afford us a consistent experience. |
@jupierce it works for me, but i'm not the one who needs to be convinced :) |
Showing meta.name at least once is important, and what you've described is On Tue, Jul 19, 2016 at 9:18 AM, Justin Pierce [email protected]
|
it is being displayed at least once, so i'm not clear on your objection? |
Latest commit: With display-name and message.
Removing message:
Removing display-name:
Removing description:
|
@smarterclayton How is the latest output looking to you? |
If a template author has a really good description AND a message, do
we want to show both? If we only show one, it means the web UI sees
both but the cli only sees one. I don't think we want to encourage
authors to copy their description into their message.
|
How about:
Where "---------" would only be included if (message||parameter) existed. Which would yield something like:
|
No, we don't put descriptions on the title line because they can be very <display-name|meta.name>[description] [message] No less than 1 empty line between description and message On Fri, Jul 22, 2016 at 12:59 PM, Justin Pierce [email protected]
|
Sure thing! Here is the latest: With display-name, description, and message:
With display-name, description:
With only description:
With no explanatory metadata:
|
Looks good On Fri, Jul 22, 2016 at 5:16 PM, Justin Pierce [email protected]
|
[merge] |
[Test]ing while waiting on the merge queue |
flake #9457 |
[merge] #9457 On Mon, Jul 25, 2016 at 2:40 PM, OpenShift Bot [email protected]
|
[merge] #9680 On Mon, Jul 25, 2016 at 4:50 PM, OpenShift Bot [email protected]
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/6875/) (Image: devenv-rhel7_4667) |
[test] secret mount issue
|
[test] applied cluster resource quota On Mon, Jul 25, 2016 at 9:35 PM, OpenShift Bot [email protected]
|
Evaluated for origin test up to 5c06abe |
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/6875/) |
[merge] |
Evaluated for origin merge up to 5c06abe |
To continue debate on #9708
@bparees @smarterclayton @deads2k @liggitt @jorgemoralespou