Adding a field to templates to allow them to deliver a user message w… #9708

jupierce · 2016-07-05T18:41:12Z

…ith parameters substituted

jupierce · 2016-07-05T19:04:55Z

Consider the following template: https://gist.github.com/jupierce/4ce49e0f46f8647d798c3db5b6fa42e9

Uses a field called "PostProcessMessage" in template to store/display the parameterized end user message
And new-app output looks like: https://gist.github.com/jupierce/507bca3ecac23ffecb3161cbd2f73785

In addition to new-app changes, the feature seemed to call for the PostProcessMessage to be displayed after "oc process", but the existing output of 'process' does not lend itself to that behavior. "oc process" presently displays the list of objects extracted from the transformed template instead of the full template: https://gist.github.com/jupierce/74dab02e628f80d55fc9d94c8086dc9a

I could see us doing any of the following for oc process:

Leave "oc process" as-is and only display the post process message as part of new-app and through the web console
Change "oc process" so that it outputs the full transformed template instead of just a list of objects. This would mean "oc create" would need to be able to interpret a full, already transformed, template as input, and extract/display the post process message. It would also cause ripples through any codebase using oc process output directly.
Include the post process message as an annotation in the "oc process" list output. "oc create" could then have a heuristic that looked for the message and displayed it after processing the list.

jupierce · 2016-07-05T19:07:52Z

Please let me know your thoughts @bparees @smarterclayton

bparees · 2016-07-05T19:25:31Z

pkg/template/api/types.go

@@ -13,6 +13,9 @@ type Template struct {
 	unversioned.TypeMeta
 	kapi.ObjectMeta

+	// Optional: An instructional message for the user processing this template (escaped parameters will be substitued).


escaped parameters?

Changing to "An instructional message for the user processing this template (parameter substitution will be performed)." for clarity.

how about: "An instructional message for the user that will be displayed when the template is instantiated (parameter substitution will be performed)."

(technically it is true that the message is handled during processing, but the only clients that are going to display the message are clients that are doing actual instantiation. If the user just runs "oc process" on the template, the message will not be explicitly displayed).

bparees · 2016-07-05T19:54:11Z

a few nits/open question but mostly looks good. please update one of the existing tests to also validate this new logic (hopefully we have an appropriate one that does parameter substitution testing already).

liggitt · 2016-07-05T21:49:04Z

api/swagger-spec/oapi-v1.json

@@ -24841,6 +24841,10 @@
      "$ref": "v1.ObjectMeta",
      "description": "Standard object's metadata."
     },
+     "postProcessMessage": {
+      "type": "string",
+      "description": "Optional: An instructional message for the user processing this template (escaped parameters will be substitued)."


any good go spell-checkers out there? :-)

not to worry, we have a large contingent of high-functioning OCD sufferers in this org.

I think you mean CDO (alphabetized, like it should be)

jupierce · 2016-07-06T12:46:44Z

@bparees

Test added
API guidance improved

liggitt · 2016-07-06T13:57:21Z

pkg/cmd/cli/describe/describer.go

@@ -946,6 +955,8 @@ func (d *TemplateDescriber) DescribeTemplate(template *templateapi.Template) (st
 		formatMeta(out, template.ObjectMeta)
 		out.Write([]byte("\n"))
 		out.Flush()
+		d.DescribePostProcessMessage(template.PostProcessMessage, out)


the parameters seem more important than the post process message, and the message is likely to be longish, so I'd put it after the params

jorgemoralespou · 2016-07-06T16:34:28Z

@bparees @jupierece As a good developer that I am I usually just look at the end of the output of new-app so I would move this mage right to the end to make it easier to catch attention of the user.

jupierce · 2016-07-06T16:47:51Z

@jorgemoralespou That was definitely my intent at the outset; however, new-app has two characteristics which pointed me to the current implementation:
(1) new-app can process multiple templates during a single invocation. If message output was deferred until the end of the process, the resultant messages might no longer be contextually tied to the template which created them.
(2) Moving messages to the end of new-app processing would require a much more invasive change to the current implementation.

bparees · 2016-07-06T16:51:02Z

+1 @jupierce, was going to say something similar. you could also new-app multiple things, some of which are templates and some are not.

smarterclayton · 2016-07-06T17:52:25Z

pkg/template/api/v1/types.go

-	// Objects is an array of objects to include in this template. Required.
+	// PostProcessMessage is an optional instructional message for the user
+	// processing this template (parameter substitution will be performed).
+	PostProcessMessage string `json:"postProcessMessage,omitempty"`


PostProcess is too technical, I'd want someone who is a brand new user to figure it out. Message is probably sufficient.

Add a longer description that captures why someone would use this.

Use the lowercase names in godoc so the messages in swagger match the JSON

smarterclayton · 2016-07-06T17:55:46Z

We already need to do better error detection in new-app (for instance, new app should not run without --confirm if objects already exist). So in the future we'll have a better idea of whether we'll fail or not before we do the actual processing. We're not optimizing for the multiple template case, but we should be aware of it. If the experience is better for an end user on the single template case if it's at the end, that's where we should show it.

bparees · 2016-07-06T23:12:39Z

If the experience is better for an end user on the single template case if it's at the end, that's where we should show it.

it's better until:

we make the new-app code so messy trying to move that line of output to the end that we can't easily fix the next issue/feature that is requested for it or the next person to touch the code has to spend extra hours figuring out what is going on because it's not logically grouped anymore. that hurts user experience too.
someone instantiates a template that provides a useless message and then tells us "hey, it'd be better if you told me that a buildconfig got created as the last message so at least i'd know to start it, instead of the last message being this useless message the user provided. We can't expect all our users provide useful messages in their templates, so that shouldn't be the most important/last thing we display"

smarterclayton · 2016-07-06T23:28:37Z

On Jul 6, 2016, at 7:12 PM, Ben Parees [email protected] wrote:

If the experience is better for an end user on the single template case if
it's at the end, that's where we should show it.

it's better until:

we make the new-app code so messy trying to move that line of output to
the end that we can't easily fix the next issue/feature that is requested
for it or the next person to touch the code has to spend extra hours
figuring out what is going on because it's not logically grouped anymore.
that hurts user experience too.

We can fix new-app. Making the code better is a time thing, but usability
matters more.

someone instantiates a template that provides a useless message and then
tells us "hey, it'd be better if you told me that a buildconfig got created
as the last message so at least i'd know to start it, instead of the last
message being this useless message the user provided. We can't expect all
our users provide useful messages in their templates, so that shouldn't be
the most important/last thing we display"

I don't think I agree. Anyone who provides a template message almost
certainly thinks their message is more important than what new-app already
displays. Our messages at the end are breadcrumbs, but they are at best
affordances to move the user to the next logical step. It's arguable that
new-app should stream build / deployment output to you rather than write
the message. I might even argue we should suppress the list of things the
template creates in favor of a user generated message.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#9708 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/ABG_p5-mG7dCHljA5vMJO4OOG7X6538Mks5qTDZpgaJpZM4JFaEm
.

bparees · 2016-07-06T23:47:23Z

I don't think I agree. Anyone who provides a template message almost
certainly thinks their message is more important than what new-app already
displays.

i fully expect a significant portion will contain "this is my template message" or whatever content we put in a sample template that they copied. or "creates a foo and a bar"

i don't think you can assume that just because the template message field is non empty, it's therefore properly spec'd and useful.

and in any case my argument is primarily one of expediency. We have finite time, this isn't worth the refactoring effort/code complexity. (particularly in light of this being turned into some sort of magic special case that only happens when we're instantiating a single template, because for any other case it's absolutely the wrong thing to do).

bparees · 2016-07-07T00:04:31Z

in case it is still not clear: i am not disagreeing that for the single scenario of "i'm using new-app to instantiate a single template that has a useful message and nothing else" printing the message last is preferable.

I am arguing that it's not worth the code churn, particularly in light of all the cases in which is not useful which we'd either have to special case out (more code churn), or accept resulted in a worse experience for those cases.

smarterclayton · 2016-07-07T00:29:45Z

I don't, and we aren't designing for that. We do not add features and then
say "users will not use these right, so let's just have a crappy feature".
We figure out how the feature enables the user, and then we support
enabling it.

On Wed, Jul 6, 2016 at 7:47 PM, Ben Parees [email protected] wrote:

I don't think I agree. Anyone who provides a template message almost
certainly thinks their message is more important than what new-app already
displays.

i fully expect a significant portion will contain "this is my template
message" or whatever content we put in a sample template that they copied.
or "creates a foo and a bar"

i don't think you can assume that just because the template message field
is non empty, it's therefore properly spec'd and useful.

and in any case my argument is primarily one of expediency. We have finite
time, this isn't worth the refactoring effort/code complexity.
(particularly in light of this being turned into some sort of magic special
case that only happens when we're instantiating a single template, because
for any other case it's absolutely the wrong thing to do).

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#9708 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABG_p_KYBR0q8M4rQEG_0yrYpz4RqeABks5qTD6NgaJpZM4JFaEm
.

smarterclayton · 2016-07-07T00:30:42Z

I'm not a big fan of "well, we didn't have much time, so we did a hack job
that we'll have to fix later", as you well know.

On Wed, Jul 6, 2016 at 8:29 PM, Clayton Coleman [email protected] wrote:

I don't, and we aren't designing for that. We do not add features and
then say "users will not use these right, so let's just have a crappy
feature". We figure out how the feature enables the user, and then we
support enabling it.

On Wed, Jul 6, 2016 at 7:47 PM, Ben Parees [email protected]
wrote:

I don't think I agree. Anyone who provides a template message almost
certainly thinks their message is more important than what new-app already
displays.

i fully expect a significant portion will contain "this is my template
message" or whatever content we put in a sample template that they copied.
or "creates a foo and a bar"

i don't think you can assume that just because the template message field
is non empty, it's therefore properly spec'd and useful.

and in any case my argument is primarily one of expediency. We have
finite time, this isn't worth the refactoring effort/code complexity.
(particularly in light of this being turned into some sort of magic special
case that only happens when we're instantiating a single template, because
for any other case it's absolutely the wrong thing to do).

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#9708 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABG_p_KYBR0q8M4rQEG_0yrYpz4RqeABks5qTD6NgaJpZM4JFaEm
.

bparees · 2016-07-07T00:43:38Z

I'm not a big fan of "well, we didn't have much time, so we did a hack job
that we'll have to fix later", as you well know.

The current implementation is not a "hack job", it positions the output in the same location as the other output related to the template that was processed..

And i'm not a big fan of "we spent 3 days moving a line of text down slightly in the output instead of fixing 3 other issues in the product, and in the process we ensured that the next time we have to touch this code, it's going to consume extra time for someone because now it's more complicated than it needed to be"

here are 21 open CLI issues that contain the word "new-app" that we could work on instead of reworking this implementation because we don't think our users are capable of reading 10 lines of text:
https://github.com/openshift/origin/issues?q=is%3Aissue+is%3Aopen+new-app+label%3Acomponent%2Fcli

bparees · 2016-07-07T00:49:13Z

regarding oc process, yes it should be left as is, it's primary use case is to be piped into oc create or oc apply, so it does not make sense to have it outputting this message. The message is intended for new-app instantiation and web console instantiation. oc process is definitely an advanced user flow at this point.

smarterclayton · 2016-07-07T01:15:05Z

If we add a message intended for end users, and we put it in the wrong
place, that can sometimes have the result of being worse than doing
nothing at all. Let's talk tomorrow.

On Wed, Jul 6, 2016 at 8:49 PM, Ben Parees [email protected] wrote:

regarding oc process, yes it should be left as is, it's primary use case
is to be piped into oc create or oc apply, so it does not make sense to
have it outputting this message. The message is intended for new-app
instantiation and web console instantiation. oc process is definitely an
advanced user flow at this point.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#9708 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABG_p3Bv3rLk_jfImiOrOoqhEkniDYK5ks5qTE0NgaJpZM4JFaEm
.

bparees · 2016-07-07T01:24:19Z

i think we should split the new-app updates out of this PR so we can move forward w/ the api+parameter substitution changes, regardless of what we decide to do w/ the new-app output.

that way @jwforres can pick up this change for the web console sooner than later.

jupierce · 2016-07-07T13:30:27Z

Changes:
(1) Changed from PostProcessMessage to Message in template definition.
(2) Improved description of field.
[test]

bparees · 2016-07-07T13:34:30Z

lgtm, can you remove the new-app change for now, we'll get api sign-off and merge this, and then @smarterclayton and I will resume our battle royale in your follow-up PR?

jupierce · 2016-07-07T14:18:27Z

New commit removes changes from new-app.

As a new user of the system I will throw in my impression: it doesn't seem like I should have needed to use new-app at all. I would have expected an incarnation of "oc create" to allow me to process a local template file and the user message to be printed as its last step.
Honestly, even that "oc create -f <template.json>" created a Template resource instead of just instantiating the declared objects was counter-intuitive.

deads2k · 2016-07-07T15:18:07Z

pkg/template/api/v1/types.go

@@ -14,15 +14,23 @@ type Template struct {
 	// Standard object's metadata.
 	kapi.ObjectMeta `json:"metadata,omitempty"`

-	// Objects is an array of objects to include in this template. Required.
+	// message is an optional instructional message that will
+	// be displayed when this template is instantiated.


What do you mean by instantiated? You can't output this message in the oc process template | oc create -f flow, right? We're adding a template API field for the the new-app only flow? @jwforres would need to special case this in the webconsole too.

@deads2k yes this is being added to the webconsole, @jwforres is aware of it. in fact it's the primary use case for adding this.

would you be happier with "optional message that may be displayed depending which client you use and whether it bothers to support this field"?

would you be happier with "optional message that may be displayed depending which client you use and whether it bothers to support this field"?

Yeah, along with, "in our default client, we ignore this in the default processing/creation workflow".

our default client is new-app and it's not going to ignore it.

our default client is new-app and it's not going to ignore it.

As opposed the oc process command we built specifically to instantiate templates?

correct. oc process got deprecated when @smarterclayton decided to add template processing to new-app.

oc process is for advanced users who want to pipe to oc apply or something else.

Instantiated - my initial wording was "processed", but it was suggested that the only reason we process templates is to instantiate the underlying objects in etcd.

Correct, the present implementation does not attempt to output the message during "oc process" (or create). Only targeting new-app and the web console with @jwforres 's help.

bparees · 2016-07-07T15:23:31Z

oc create is not going to instantiate a template, because oc create creates resources, and a template definition is a resource, so oc create template will always create a template resource, not instantiate them. I agree it's slightly confusing to a new user to distinguish between "creating a template" (which we called "registering" or "installing" it) and instantiating a template, but I don't see us special casing oc create behavior in the case of a template resource because you'd still need a way to declare your intention to register the template when that is what you want to do.

deads2k · 2016-07-07T15:28:03Z

special casing oc create behavior in the case of a template resource

Neither do I. I would picture having an openshift.io/creation-message annotation supported on every object that has an ObjectMeta.

deads2k · 2016-07-07T15:38:28Z

Having it on individual objects would also give a spot for anyone at any later date to have a field that amounts to a description of what the resource actually is or is trying to do. Putting it on a template instead means you get it once and you better pay attention.

jupierce · 2016-07-07T15:39:14Z

@bparees Completely agree on preserving the behavior of "oc create -f" to allow advanced users to create a Template resource. Merely suggesting a short-cut for new users:
oc create [templateFile.json]

liggitt · 2016-07-07T15:40:02Z

Having it on individual objects would also give a spot for anyone at any later date to have a field that amounts to a description of what the resource actually is or is trying to do. Putting it on a template instead means you get it once and you better pay attention.

interesting, though I wouldn't want a getting started message that included a substituted in admin username and generated password to be persisted as an object annotation

deads2k · 2016-07-07T15:42:11Z

interesting, though I wouldn't want a getting started message that included a substituted in admin username and generated password to be persisted as an object annotation

You could do worse than, "... (default username/password is admin/admin". Also, I'd expect people to be at least trying to use their SA as an oauth client, so it would be "... login with your openshift credentials".

bparees · 2016-07-07T15:44:17Z

@deads2k: can you take this topic to another issue/proposal?

deads2k · 2016-07-07T15:45:32Z

@deads2k: can you take this topic to another issue/proposal?

I see it as a replacement for this pull, not an addition to it. Why would I want this feature if I had descriptions or creation messages on any API object and the existing template substitution logic?

bparees · 2016-07-07T15:53:15Z

because what you're proposing is way more tedious for a template author.

"this template creates a route with hostname $HOST}, your login is ${USER} and your password is ${PASSWORD}. You should go login and create a regular user account for yourself".

vs putting all that stuff on random resources within the template and who knows how it gets displayed to the user.

deads2k · 2016-07-07T15:57:22Z

because what you're proposing is way more tedious for a template author.

"this template creates a route with hostname $HOST}, your login is ${USER} and your password is ${PASSWORD}. You should go login and create a regular user account for yourself".

vs putting all that stuff on random resources within the template and who knows how it gets displayed to the user.

Do you have a better example? That just turns into an annotation on the route:

"Exposes a <blank> server at $HOST with default credentials user/password. You should login and create a regular user account"

Looks like the same amount of work.

bparees · 2016-07-07T16:59:45Z

you haven't solved the problem of how all these random annotations get presented to the user in a cohesive way.

smarterclayton · 2016-07-07T16:59:56Z

Let's have an in person discussion.

smarterclayton · 2016-07-11T16:53:21Z

pkg/cmd/cli/describe/describer.go

@@ -870,6 +870,15 @@ type TemplateDescriber struct {
 	kctl.ObjectDescriber
 }

+// DescribePostProcessMessage prints the message that will be parameter substituted and displayed to the
+// user when this template is processed.
+func (d *TemplateDescriber) DescribePostProcessMessage(msg string, out *tabwriter.Writer) {


Rename method

Renamed to DescribeMessage(...)

smarterclayton · 2016-07-11T16:53:49Z

API approved but one minor code change

…ith parameters substituted

bparees · 2016-07-11T17:54:29Z

pkg/template/api/v1/types.go

+	// Parameter substitution will be performed on the message before being
+	// displayed so that generated credentials and other parameters can be
+	// included in the output.
+	Message string `json:"message,omitempty"`


need to start this godoc w/ "message"

@bparees It appears to in my view of the commit. There are several inline comments that split the full godoc in half in the github diff view.

whoops, you're right, it was just such a natural start to the comment I didn't think to scroll up, my bad.

bparees · 2016-07-11T17:55:19Z

one nit and lgtm.

openshift-bot · 2016-07-11T18:01:18Z

Evaluated for origin test up to 34a3c56

bparees · 2016-07-11T19:20:41Z

[merge]

openshift-bot · 2016-07-11T19:25:44Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/6071/) (Image: devenv-rhel7_4567)

openshift-bot · 2016-07-11T19:25:45Z

Evaluated for origin merge up to 34a3c56

openshift-bot · 2016-07-11T19:30:53Z

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/6054/)

bparees reviewed Jul 5, 2016
View reviewed changes

liggitt reviewed Jul 5, 2016
View reviewed changes

liggitt reviewed Jul 6, 2016
View reviewed changes

smarterclayton reviewed Jul 6, 2016
View reviewed changes

smarterclayton added the needs-api-review label Jul 6, 2016

deads2k reviewed Jul 7, 2016
View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 11, 2016

smarterclayton reviewed Jul 11, 2016
View reviewed changes

smarterclayton added api-approved and removed needs-api-review labels Jul 11, 2016

Adding a field to templates to allow them to deliver a user message w…

34a3c56

…ith parameters substituted

bparees reviewed Jul 11, 2016
View reviewed changes

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 11, 2016

openshift-bot merged commit 1c784e0 into openshift:master Jul 11, 2016

jupierce mentioned this pull request Jul 12, 2016

Adding template instructional message to new-app output #9806

Merged

jupierce mentioned this pull request Aug 3, 2016

Adding template message documentation openshift/openshift-docs#2601

Closed

Adding a field to templates to allow them to deliver a user message w… #9708

Adding a field to templates to allow them to deliver a user message w… #9708

Conversation

jupierce commented Jul 5, 2016

jupierce commented Jul 5, 2016 • edited Loading

jupierce commented Jul 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees commented Jul 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jupierce commented Jul 6, 2016

Choose a reason for hiding this comment

jorgemoralespou commented Jul 6, 2016

jupierce commented Jul 6, 2016

bparees commented Jul 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Jul 6, 2016

bparees commented Jul 6, 2016

smarterclayton commented Jul 6, 2016

bparees commented Jul 6, 2016

bparees commented Jul 7, 2016

smarterclayton commented Jul 7, 2016

smarterclayton commented Jul 7, 2016

bparees commented Jul 7, 2016

bparees commented Jul 7, 2016

smarterclayton commented Jul 7, 2016

bparees commented Jul 7, 2016

jupierce commented Jul 7, 2016

bparees commented Jul 7, 2016

jupierce commented Jul 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees Jul 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees commented Jul 7, 2016

deads2k commented Jul 7, 2016

deads2k commented Jul 7, 2016

jupierce commented Jul 7, 2016 • edited Loading

liggitt commented Jul 7, 2016

deads2k commented Jul 7, 2016

bparees commented Jul 7, 2016

deads2k commented Jul 7, 2016

bparees commented Jul 7, 2016

deads2k commented Jul 7, 2016

bparees commented Jul 7, 2016

smarterclayton commented Jul 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Jul 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees commented Jul 11, 2016

openshift-bot commented Jul 11, 2016

bparees commented Jul 11, 2016

openshift-bot commented Jul 11, 2016 • edited Loading

openshift-bot commented Jul 11, 2016

openshift-bot commented Jul 11, 2016

jupierce commented Jul 5, 2016 •

edited

Loading

bparees commented Jul 6, 2016 •

edited

Loading

bparees Jul 7, 2016 •

edited

Loading

jupierce commented Jul 7, 2016 •

edited

Loading

openshift-bot commented Jul 11, 2016 •

edited

Loading