Add dotnet-pgsql-persistent template #27

tmds · 2016-12-22T13:28:32Z

No description provided.

jerboaa · 2016-12-22T14:36:56Z

How would I test this? Could you please provide some documentation as to how this is intended to be used?

jerboaa · 2016-12-22T15:30:38Z

[skipci]

tmds · 2016-12-22T15:47:22Z

How would I test this?

No idea. Do you know how the other templates are tested?

Could you please provide some documentation as to how this is intended to be used?

Templates are supposed to be self-documenting. So give it a try, and you perhaps you find some places where the documentation can improve.

jerboaa · 2016-12-22T16:04:30Z

Please don't assume users will know how to use it. I disagree on the self-documenting note. I'm sure you've run some commands/steps to test this. Documenting them here for others to reproduce would be nice.

What other templates are you referring to? We currently only have an imagestream file.

tmds · 2016-12-22T16:15:07Z

Please don't assume users will know how to use it. I disagree on the self-documenting note. I'm sure you've run some commands/steps to test this. Documenting them here for others to reproduce would be nice.

What other templates are you referring to? We currently only have an imagestream file.

This is an OpenShift template (https://docs.openshift.org/latest/dev_guide/templates.html).

It is similar to other templates OpenShift provides out of the box: cakephp-mysql-persistent, rails-pgsql-persistent, dancer-mgsql-persistent, ...

jerboaa · 2016-12-22T16:44:16Z

General comment:

Please consider this from the point of a reviewer. For example, I as a reviewer I first look at the title and read the description next. My experience here was:

Title: Add dotnet-pgsql-persistent template
Description: <none>

At that point I'm already not super eager to look any further. We should aim at making work easier for reviewers, not harder. The suggestions for future PRs is:

Make an effort to produce good summaries and descriptions. Examples:
https://lkml.org/lkml/2016/12/22/310
add affirmative output to oc policy / oadm policy openshift/origin#12324
Add a note how this can be tested and/or what you've done to test it.

tmds · 2016-12-22T16:53:02Z

@jerboaa my bad, I thought the title would be enough for you to figure out what it was about.

I tried this with the published dotnet:1.1 and a branch that works around the various issues we have fixed in the meanwhile.

tmds · 2017-01-10T12:24:57Z

@jerboaa can you review this?

jerboaa · 2017-01-11T14:27:00Z

@tmds I can review it, but not sure when. If somebody else could have a look meanwhile it would be helpful.

Both work, but http is more correct

jerboaa · 2017-01-13T15:50:22Z

README.md

+The `templates` folder contains OpenShift templates. Some of these will be shipped with OpenShift. If a template is not on your OpenShift installation, you can import it:
+
+```
+oc create -f <template.json>


This only describes how to import the template itself. It would be useful to describe how a user would be able to instantiate it. Something like:

oc process dotnet-pgsql-persistent | oc create -f -

Once the above has been performed, what is the user expected to get? Would they be able to access the musicstore app directly? How would they be able to access it. I suppose this should go to https://github.com/redhat-developer/s2i-aspnet-musicstore-ex#openshift-web-console (i.e. into the readme there). If we add it there, we should put some references to the s2i-aspnet-musicstore-ex repo.

jerboaa · 2017-01-13T15:51:46Z

templates/dotnet-pgsql-persistent.json

+    "kind": "Template",
+    "apiVersion": "v1",
+    "metadata": {
+        "name": "dotnet-pgsql-persistent",


I wonder if we should give this a more descriptive name. How about dotnet-musicstore-pgsql-persistent? A bit longer, but seems to tell you a bit more what you're going to get. Have you considered adding a *-ephemeral template. It would be easier to test. Right now I don't have an OpenShift instance with persistent volumes set up. That makes testing harder for me ;-)

jerboaa · 2017-01-27T11:07:11Z

templates/dotnet-pgsql-persistent.json

+                    "sourceStrategy": {
+                        "from": {
+                            "kind": "ImageStreamTag",
+                            "namespace": "${NAMESPACE}",


We should remove this namespace since this prevents users from using this template if the dotnet images come from an imagestream in the local (current) namespace and don't have it in openshift.

Is this different for the rails example? I got this from their template: https://github.com/openshift/rails-ex/blob/master/openshift/templates/rails-postgresql-persistent.json#L102

They assume it's preloaded, I suppose. Which they probably do for all sclorg images. Rails is one of them. We should not (yet) assume that. It should work either way if we omit the namespace.

This environment variable is used by the s2i assemble script.

tmds · 2017-01-29T16:55:23Z

DOTNET_RESTORE_ROOT must be added to the template to reduce the restore time.

This parameter is mapped to the corresponding s2i-dotnetcore environment variable

Used to specify the namespace of the .NET builder image. The postgresql image is always retrieved from the openshift namespace.

jerboaa · 2017-02-14T13:41:29Z

[skipci]

Old review

jerboaa · 2017-02-15T15:46:49Z

templates/dotnet-pgsql-persistent.json

+                                    "limits": {
+                                        "memory": "${MEMORY_LIMIT}"
+                                    }
+                                }


Could you please add a readiness/liveness probe like this to the template? This avoids a message in the webconsole showing up that the deployment does not have one.

"livenessProbe": { "httpGet": { "path": "/Store", "port": 8080, "scheme": "HTTP" }, "initialDelaySeconds": 40, "timeoutSeconds": 10, "periodSeconds": 10, "successThreshold": 1, "failureThreshold": 3 }, "readinessProbe": { "httpGet": { "path": "/Store", "port": 8080, "scheme": "HTTP" }, "initialDelaySeconds": 10, "timeoutSeconds": 30, "periodSeconds": 10, "successThreshold": 1, "failureThreshold": 3 },

jerboaa · 2017-02-24T10:59:01Z

This looks OK to me, fwiw. @tmds feel free to merge.

tmds · 2017-02-27T12:24:23Z

@jerboaa Let's merge #60 first. I'll handle the merge conflict on this PR and update the README to describe this template too.

…o feature/template

jerboaa · 2017-03-09T10:49:39Z

Do you mind adding some sample usage of the template here and reference in README.md (of this repo) like I did with #64:
https://github.com/redhat-developer/s2i-aspnet-musicstore-ex/blob/rel/1.1.0-preview1/README.md

Note there are also references to gpiercey which should be replaced by redhat-developer. Thanks!

jerboaa · 2017-03-09T12:53:07Z

README.md

+oc new-app --template=<template>
+```
+
+In case you imported the template in your project, set the `NAMESPACE` parameter to your project name


Not sure if that's correct. NAMESPACE needs to be specified when the dotnet imagestreams are not in openshift, but in a local project.

Suggested wording:

In case you've imported the .NET Core imagestreams into your local project, NAMESPACE needs to be specified to match it. This can be done by setting the NAMESPACE parameter via -p NAMESPACE=<project>.

Right. I will remove the paragraph since the README isn't describing how to import image streams.

jerboaa · 2017-03-09T12:55:16Z

README.md

+by adding the `-p NAMESPACE=<project>` parameter.
+
+The template can also be instantiated using the OpenShift web console. Login to the console and
+navigate to the desired project. Click the **Add to Project** button. Search and select the \<template>.


I'm not sure whether \<template> is very clear. How about this instead?

"Search and select the desired template by it's name, for example dotnet-example."

tmds · 2017-03-09T14:21:56Z

@jerboaa can you do a test? When the app starts it should say Using data provider: npgsql which means it is connecting to PostgreSQL.
I did a test before I renamed the branch and added the print statement.

jerboaa · 2017-03-09T15:42:46Z

@tmds working on it.

jerboaa · 2017-03-09T16:21:56Z

@tmds Works fine for me. Ship it!

tmds · 2017-03-09T16:49:33Z

Thanks @jerboaa

Add dotnet-pgsql-persistent template

25fab88

Replace https by http in ASPNETCORE_URLS

cc3ebf5

Both work, but http is more correct

jerboaa previously requested changes Jan 17, 2017

View reviewed changes

jerboaa reviewed Jan 27, 2017

View reviewed changes

Add DOTNET_STARTUP_PROJECT to BuildConfig env

6f64e18

This environment variable is used by the s2i assemble script.

Add DOTNET_RESTORE_ROOT template parameter

4123a24

This parameter is mapped to the corresponding s2i-dotnetcore environment variable

jerboaa mentioned this pull request Jan 30, 2017

Remove .s2i/environment redhat-developer/s2i-aspnet-musicstore-ex#4

Merged

tmds added 4 commits February 6, 2017 11:00

Remove ASPNETCORE_URLS (set by s2i-dotnetcore)

7a162ee

DOTNET_STARTUP_PROJECT is only used in Build, remove from Deployment

beb85c6

Use samples/MusicStore as context-root

0b67f51

Remove namespace (PR feedback)

10f4cc8

tmds mentioned this pull request Feb 8, 2017

Set DOTNET_ASSEMBLY_NAME to be able to use samples/MusicStore as context-dir redhat-developer/s2i-aspnet-musicstore-ex#6

Merged

tmds added 2 commits February 9, 2017 16:39

Remove DOTNET_RESTORE_ROOT (cfr redhat-developer#48)

b832baa

Add NAMESPACE parameter back

a335f75

Used to specify the namespace of the .NET builder image. The postgresql image is always retrieved from the openshift namespace.

jerboaa reviewed Feb 15, 2017

View reviewed changes

tmds added 2 commits February 21, 2017 09:24

Add livenessProbe and readinessProbe to dotnet container

b090909

Default NAME parameter to musicstore

14033af

tmds added 7 commits March 9, 2017 10:47

Merge branch 'master' into feature/template

1193f66

Adjust for 1.1.1-example branch

771224b

Add all s2i-dotnetcore build envvars

d975498

Merge remote-tracking branch 'upstream/master' into feature/template

d309942

Merge branch 'feature/template' of github.com:tmds/s2i-dotnetcore int…

a8c7c81

…o feature/template

Update README

8805f00

Update git ref

9f5acfc

Add template usage instructions

4872273

jerboaa reviewed Mar 9, 2017

View reviewed changes

tmds added 3 commits March 9, 2017 14:01

PR feedback

bc1658d

Add annotations

0f9d807

rel/1.1.1-example -> rel/1.1-example

21bb483

tmds merged commit b84be6d into redhat-developer:master Mar 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add dotnet-pgsql-persistent template #27

Add dotnet-pgsql-persistent template #27

tmds commented Dec 22, 2016

jerboaa commented Dec 22, 2016

jerboaa commented Dec 22, 2016

tmds commented Dec 22, 2016

jerboaa commented Dec 22, 2016

tmds commented Dec 22, 2016

jerboaa commented Dec 22, 2016

tmds commented Dec 22, 2016

tmds commented Jan 10, 2017

jerboaa commented Jan 11, 2017

jerboaa Jan 13, 2017

jerboaa Jan 13, 2017

jerboaa Jan 27, 2017

tmds Jan 27, 2017

jerboaa Jan 27, 2017

tmds commented Jan 29, 2017

jerboaa commented Feb 14, 2017

jerboaa Feb 15, 2017

jerboaa commented Feb 24, 2017

tmds commented Feb 27, 2017

jerboaa commented Mar 9, 2017

jerboaa Mar 9, 2017

tmds Mar 9, 2017

jerboaa Mar 9, 2017

tmds commented Mar 9, 2017

jerboaa commented Mar 9, 2017

jerboaa commented Mar 9, 2017

tmds commented Mar 9, 2017

Add dotnet-pgsql-persistent template #27

Add dotnet-pgsql-persistent template #27

Conversation

tmds commented Dec 22, 2016

jerboaa commented Dec 22, 2016

jerboaa commented Dec 22, 2016

tmds commented Dec 22, 2016

jerboaa commented Dec 22, 2016

tmds commented Dec 22, 2016

jerboaa commented Dec 22, 2016

tmds commented Dec 22, 2016

tmds commented Jan 10, 2017

jerboaa commented Jan 11, 2017

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

tmds commented Jan 29, 2017

jerboaa commented Feb 14, 2017

Choose a reason for hiding this comment

jerboaa commented Feb 24, 2017

tmds commented Feb 27, 2017

jerboaa commented Mar 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmds commented Mar 9, 2017

jerboaa commented Mar 9, 2017

jerboaa commented Mar 9, 2017

tmds commented Mar 9, 2017