Minor improvements to voting-app and webserver #5

lindydonna · 2017-12-15T09:05:31Z

Simplified code in voting-app and added comments on what each part is doing
Added 2 other versions of webserver for each stage of the quickstart (simple, add component, add availability zones)
Fixed tsconfig to target only Node libraries (fixes part of https://github.com/pulumi/home/issues/88)

lukehoban

Few minor comments.

lukehoban · 2017-12-15T21:02:53Z

webserver-component/package.json

+        "pulumi": "*"
+    },
+    "dependencies": {
+        "@types/node": "^8.0.26"


Minor - but this can be a devDependencies

lukehoban · 2017-12-15T21:03:35Z

webserver-component/tsconfig.json

+        "strictNullChecks": true
+    },
+    "files": [
+        "index.ts"


Technically we should include webserver.ts here too I think. I believe this won't cause problems for most tools - but may as well add it.

lukehoban · 2017-12-15T21:41:21Z

webserver/index.ts

+});
+
+server.publicDns.then(dns => console.log(`Server URL: http://${dns}`));


export let here as well?

👍

Especially if we can somehow avoid needing to explain http://undefined.

Unfortunately, I need to explain http://undefined later, because of the availability zone example and async. Full quickstart flow is here: https://github.com/pulumi/docs/blob/master/quickstart/aws.md or https://docs.pulumi.com/quickstart/aws.html

That said, I should certainly introduce export right away, because it's an awesome feature.

joeduffy · 2017-12-15T22:07:51Z

.vscode/tasks.json

@@ -0,0 +1,59 @@
+{
+    // See https://go.microsoft.com/fwlink/?LinkId=733558


Eek, does VS Code really emit invalid JSON? 😳

Can we add .vscode to the .gitignore file?

This was intentional, so that people can build each project easily in vscode. If you'd prefer I remove it, I can.

Comments in JSON are technically not valid, but many JSON parsers accept them.

I'd remove it. The pedant in me just cant stand to have illegal JSON in our repos.

If I remove the comments, are you ok with the file being there? It is actually useful for people who use VSCode, as it sets up build tasks for each project.

joeduffy · 2017-12-15T22:08:34Z

voting-app/index.ts

@@ -4,36 +4,41 @@ import * as cloud from "@pulumi/cloud";

 // To simplify this example, we have defined the password directly in code
 // In a real app, would add the secret via `pulumi config secret <key> <value>` and
-// access via config APIs
+// access via pulumi.Config APIs
 let redisPassword = "SECRETPASSWORD"; 


Definitely would love to see this as a secret config variable. Else, it'll make people think we don't take security seriously, or that our product doesn't have the features required ... which it does!

Indeed! I'll fix this when I start the bigger doc overhaul.

Ideally, I would change it so you add the secret in a later step, to simplify the flow. The problem is that this sample takes 20 minutes to deploy. If I change an environment variable in the docker container, what needs to be rebuilt?

joeduffy · 2017-12-15T22:09:53Z

webserver-component/index.ts

+import * as webserver from "./webserver"; // use the new custom component
+
+let webInstance = webserver.createInstance("web-server-www", "t2.micro");
+let appInstance = webserver.createInstance("web-server-app", "t2.nano");


If we're in here, I have to admit ... at some point this changed from appInstance being a "large" to just being a "nano" ... every time I demo this, I have this inner feeling of awkwardness, because the app server is almost always beefier than the thing just serving up simple HTML. I wonder if we should change to even maybe a non-t2 instance size, something more "memory" optimized?

Agreed, but this sample is used in the quickstart and I wouldn't want to suggest people provision larger VMs that they don't use. Maybe m1.small? https://www.ec2instances.info/?selected=m1.small

Agreed with @lindydonna, I think if this is something in the quickstart we should err on the side of small instances (and add appropriate warnings that this will deploy real resources which cost real money, in your account).

For the similar example in our examples folder for tests, may make sense to use a larger size here for demoing.

I'm fine keeping them small, but we should at least reverse the sizes... this really bugs me more than it should 😄

joeduffy · 2017-12-15T22:10:39Z

webserver-component/webserver.ts

+        tags: { "Name": name },             // use the `name` parameter
+        instanceType: size,                 // use function argument for size
+        securityGroups: [ group.name ],     // reference the group object above
+        ami: aws.ec2.getLinuxAMI(size),     // call built-in function (can also be custom)


Should we consider changing this to an AMI query instead of our own homegrown getLinuxAMI?

I actually had the AMI query and I changed it back, to simplify the example. I'm going to simplify even further and remove the userData until a later step.

Also I need to change this back to JavaScript.

joeduffy · 2017-12-15T22:11:19Z

webserver/index.ts

+});
+
+server.publicDns.then(dns => console.log(`Server URL: http://${dns}`));


👍

Especially if we can somehow avoid needing to explain http://undefined.

lindydonna · 2017-12-15T23:55:01Z

Talking to Luke more, I think the Availability Zone example is not worth using in its current form, since it means we can't use export. I'll work on a example that has loops and doesn't fall into the promises pitfall.

lindydonna · 2017-12-18T23:26:25Z

I'm going to merge as-is so that I can update the docs ASAP with a link to the examples for a customer. I filed #6 to track the code improvements.

Minor improvements to voting-app and webserver

Update code to current version of SDK

77089bc

lukehoban approved these changes Dec 15, 2017

View reviewed changes

joeduffy reviewed Dec 15, 2017

View reviewed changes

lindydonna mentioned this pull request Dec 18, 2017

Address review feedback from first round of example improvements #6

Closed

6 tasks

lindydonna merged commit a1a34e9 into master Dec 18, 2017

This was referenced Dec 20, 2017

Use simplified form of getEndpoint #4

Closed

Substitute availability zone example with a better one #7

Closed

lindydonna deleted the donna-wip branch January 3, 2018 23:08

ramene pushed a commit to ramene/pulumi-kubeflow-ml that referenced this pull request Sep 7, 2019

Merge pull request pulumi#5 from pulumi/donna-wip

8e889a3

Minor improvements to voting-app and webserver

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minor improvements to voting-app and webserver #5

Minor improvements to voting-app and webserver #5

lindydonna commented Dec 15, 2017

lukehoban left a comment

lukehoban Dec 15, 2017

lukehoban Dec 15, 2017

lukehoban Dec 15, 2017

joeduffy Dec 15, 2017

lindydonna Dec 15, 2017

joeduffy Dec 15, 2017

lindydonna Dec 15, 2017

joeduffy Dec 16, 2017

lindydonna Dec 16, 2017

joeduffy Dec 15, 2017

lindydonna Dec 15, 2017

joeduffy Dec 15, 2017

lindydonna Dec 15, 2017 •

edited

lukehoban Dec 15, 2017

joeduffy Dec 16, 2017

joeduffy Dec 15, 2017

lindydonna Dec 15, 2017

joeduffy Dec 15, 2017

lindydonna commented Dec 15, 2017

lindydonna commented Dec 18, 2017

		});

		server.publicDns.then(dns => console.log(`Server URL: http://${dns}`));

		@@ -0,0 +1,59 @@
		{
		// See https://go.microsoft.com/fwlink/?LinkId=733558

Minor improvements to voting-app and webserver #5

Minor improvements to voting-app and webserver #5

Conversation

lindydonna commented Dec 15, 2017

lukehoban left a 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

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

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

lindydonna Dec 15, 2017 • edited

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

Choose a reason for hiding this comment

lindydonna commented Dec 15, 2017

lindydonna commented Dec 18, 2017

lindydonna Dec 15, 2017 •

edited