docs: Added tutorial topic #1047

Doug-AWS · 2018-10-30T20:02:19Z

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

Resolves comments from PR #872; Steve's review.

Adds a `pkglint` rule to enforce presence of the `*.snk` pattern in both the `.gitignore` and `.npmignore` file as a way to make it harder for one to accidentially push the material to public locations. This was missed in #643.

rix0rrr · 2018-10-31T14:30:27Z

docs/src/tutorial.rst

+    exports.main = function(event, context, callback) {
+      if (event.httpMethod != null) {
+        if (event.httpMethod != "") {
+          if (event.httpMethod == "GET") {


These three lines below each other make me restless. There should be a better way to express this.

I've refactored the code to use a try block.

I don't see how that helps.

What's making me nervous here, and I should have been more clear, is you're doing the following:

if (variable unequal to one thing) AND if (variable unequal to other thing) AND if (variable IS EUQAL to something else) THEN do something

Well first of all, if event.httpMethod == "GET", then of course it must be != null and != "", so there's no need to check those. You could have gotten away with writing:

if (event.httpMethod == "GET") { // ... } if (event.httpMethod == "POST") { // ... } // etc

And have the same effect.

The only reason you would type it this way, is because maybe at the end you might write an else, something like this:

if (event.httpMethod != "") { if (Event.httpMethod == "GET") { // lots of } // ...lines here... } else { // Do something special }

However, because the inner block is SO LONG, I have a hard time visually scanning whether that's true or not. So I recommend you invert the condition into an early abort, if that's the case:

if (event.httpMethod == "") { // Do something special return; } if (event.httpMethod == "GET") { /// ... } if (event.httpMethod == "POST") { // ... } // etc

Long blocks and redundant ifs make me nervous because they might be hiding logic that's easy to miss, and so I recommend that you change the code layout to be very clear and straightforward about what's happening.

I'm not sure how adding a try helps there. But also, I can't find the code back in the PR after it has exploded through the merge, so I'll have a look again after it has normalized.

Also

Forgot to take note of this first time around, but since you're doing JavaScript, REPLACE ALL INSTANCES of the following:

== => ===

!= => !==

The 2-character variants are not to be trusted.

rix0rrr · 2018-10-31T14:31:13Z

docs/src/tutorial.rst

+The next step is to create a |LAM| function to list all of the widgets in our
+|S3| bucket.
+
+Create the directory *resource* at the same level as the *bin* directory.


resource or resources?

Also is that the best name for this directory?

rix0rrr · 2018-10-31T14:35:14Z

docs/src/tutorial.rst

+    exports.main = function(event, context, callback) {
+      if (event.httpMethod == "GET") {
+        if (event.path.length == 1) {
+          S3.listObjectsV2({ Bucket: bucketName })


Use NodeJS8 and you can rewrite this to be async, which is way more readable:

exports.main = async function(event, context) { try { if (event.httpMethod == "GET") { if (event.path.length == 1) { const data = await S3.listObjectsV2({ Bucket: bucketName }).promise(); var body = { widgets: data.Contents.map(function(e) { return e.Key }) }; return { statusCode: 200, headers: {}, body: JSON.stringify(body) }; } } } catch(error) { var body = error.stack || JSON.stringify(error, null, 2); var response = makeError(body); return response; } }

By the way where does makeError come from?

Okay, I no longer use helper functions.

That's not the point. I like helper functions. I just didn't see it defined. Or did I miss it?

rix0rrr · 2018-10-31T14:36:34Z

docs/src/tutorial.rst

+
+    npm install @aws-cdk/aws-apigateway
+    npm install @aws-cdk/aws-lambda
+    npm install @aws-cdk/aws-s3


This is probably all possible on one line

rix0rrr · 2018-10-31T14:38:16Z

docs/src/tutorial.rst

+.. code-block:: sh
+
+    npm run build
+    cdk synth


What's the point of this?

I want to reinforce that when you make changes you should rebuild and see if they are what you want.

Fair enough

Encountered someone at a workshop who had copy/pasted into the wrong file, replacing their app instantiation and invocation, and the error message was "could not find cdk.out" Which was super unhelpful and took me a good while to figure out what had happened. Pinpoint at least one possible cause of this happening in the error message.

) Adds an option to specify `partitionKey` and/or `sortKey` when the table is initialized. This is identical to calling `addPartitionKey` and `addSortKey`. Fixes #1051

…-cdk into dougsch/new-tutorial

* feat(aws-sqs): improvements to IAM grants API Moved `grantXxx` methods from `Queue` to `QueueRef`, so they can now be performed on imported queues. Added commonly needed permissions to `grantConsumeMessages` and `grantSendMessages` such as `sqs:GetQueueAttributes`, `sqs:GetQueueUrl` and the various `sqs:xxxBatch` actions. Added support for adding arbitrary actions to each of the grant methods. Exposed `queue.grant(...actions)` as a general purpose grant method which allows users to customize the set of actions for this specific resource/principal pair. BREAKING CHANGE: `queue.grantReceiveMessages` has been removed. It is unlikely that this would be sufficient to interact with a queue. Alternatively you can use `queue.grantConsumeMessages` or `queue.grant('sqs:ReceiveMessage')` if there's a need to only grant this action.

rix0rrr · 2018-11-01T09:52:49Z

Something went a little weird with your merge.

Adds a uniform high-level API for AWS Lambda event sources. This API allows developers to use a single API `lambda.addEventSource(source)` to connect a function with any type of event source. The `IEventSource` class is defined in the aws-lambda module and the aws-lambda-event-sources module includes implementations for various event sources. Each implementation utilizes the relevant mechanism. Currently supported: - SQS - SNS - S3 We eventually want to support all event sources as described in the Lambda developer guide.

This change implements two asset bandwidth conservation measures: - If a lambda.AssetCode object is reused for multiple Lambdas, the same underyling Asset object will be reused (which leads to the asset data only being uploaded once). - If nonetheless multiple Asset objects are created for the same source data, the data will only be uploaded once and subsequently copied on the server-side to avoid the additional data transfer. Fixes #989.

…-cdk into dougsch/new-tutorial

Doug-AWS · 2018-11-01T15:18:33Z

WTF is going on? When I had my dougsch/new-tutorial branch checked out, I ran "git diff --name-only master" and it gave me index.rst and tutorial.rst. Now it says I have 191 file diffs?

OK, I just rebased my branch against master:

git checkout master
git pull
git checkout dougsch/new-tutorial
git pull
git fetch --all -p
git fetch origin
git rebase origin/master

Got a merge conflict in tutorial.rst. How could that happen as master does not have a tutorial.rst????
Whatever, fixed the merge conflict, then ran:

git pull
git push
git status

Gave me:

On branch dougsch/new-tutorial
Your branch is up to date with 'origin/dougsch/new-tutorial'.

nothing to commit, working tree clean

git diff --name-only master

Gave me:

docs/src/index.rst
docs/src/tutorial.rst

What else am I supposed to do???

Doug-AWS · 2018-11-01T15:19:50Z

Forget this. I'm trying a new branch, dougsch/tutorial, and new PR.

rix0rrr · 2018-11-02T09:37:20Z

Doug, I think the problem is in your rebasing.

Don't rebase!

Rebasing will try to move the commits around, change their identity, resurface merge conflicts that you've already solved, etc.

Rebasing is a good solution if you're committing directly to master and you want to keep your history clean. However, we work via PRs that we squash, so there's no need to keep the history clean. Any change goes into master as a single commit, and the history leading up to that commit is lost. Your life is going to be a lot better if you stick to the following:

# Start a new branch
(master)$ git checkout master
(master)$ git checkout -b dougsch/some-change
(dougsch/some-change)$ 

... work ...

# Ready to create a PR
(dougsch/some-change)$ git add -A .
(dougsch/some-change)$ git commit
(dougsch/some-change)$ git push -u

# Switch to another branch to continue working there
(dougsch/some-change)$ git checkout dougsch/oldpr
(dougsch/oldpr)$ git fetch 
# NO REBASE HERE!
(dougsch/oldpr)$ git merge origin/master

... work ...

# Update the PR branch
(dougsch/oldpr)$ git commit -a
(dougsch/oldpr)$ git push

If you don't yet have a script installed yet that shows your current branch in your shell prompt, get one today. Super handy!

Doug-AWS and others added 2 commits October 30, 2018 12:58

Added tutorial with comments from Elad and Steve

589fcbe

feat(pkglint): Make sure .snk files are ignored (#1049)

53c8d76

Adds a `pkglint` rule to enforce presence of the `*.snk` pattern in both the `.gitignore` and `.npmignore` file as a way to make it harder for one to accidentially push the material to public locations. This was missed in #643.

rix0rrr reviewed Oct 31, 2018

View reviewed changes

rix0rrr and others added 8 commits October 31, 2018 15:40

feat(aws-dynamodb): allow specifying partition/sort keys in props (#1054

ec87331

) Adds an option to specify `partitionKey` and/or `sortKey` when the table is initialized. This is identical to calling `addPartitionKey` and `addSortKey`. Fixes #1051

Updated based on Rico's feedback.

7746e85

Added tutorial with comments from Elad and Steve

969e0ce

Updated based on Rico's feedback.

1a93d0f

Merge branch 'dougsch/new-tutorial' of https://github.com/awslabs/aws…

9f0e8ef

…-cdk into dougsch/new-tutorial

Header for developer preview (#1057)

f55d052

Elad Ben-Israel and others added 5 commits November 1, 2018 12:42

Added tutorial with comments from Elad and Steve

5b08aed

Updated based on Rico's feedback.

071f1df

Merge branch 'dougsch/new-tutorial' of https://github.com/awslabs/aws…

ab0996a

…-cdk into dougsch/new-tutorial

Doug-AWS closed this Nov 1, 2018

Doug-AWS mentioned this pull request Nov 1, 2018

docs: Added tutorial #1065

Merged

Doug-AWS deleted the dougsch/new-tutorial branch November 1, 2018 17:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: Added tutorial topic #1047

docs: Added tutorial topic #1047

Doug-AWS commented Oct 30, 2018

rix0rrr Oct 31, 2018

Doug-AWS Oct 31, 2018

rix0rrr Nov 1, 2018

rix0rrr Oct 31, 2018

rix0rrr Oct 31, 2018

rix0rrr Oct 31, 2018

rix0rrr Oct 31, 2018

Doug-AWS Oct 31, 2018

rix0rrr Nov 1, 2018

rix0rrr Oct 31, 2018

Doug-AWS Oct 31, 2018

rix0rrr Oct 31, 2018

Doug-AWS Oct 31, 2018

rix0rrr Nov 1, 2018

rix0rrr commented Nov 1, 2018

Doug-AWS commented Nov 1, 2018

Doug-AWS commented Nov 1, 2018

rix0rrr commented Nov 2, 2018

docs: Added tutorial topic #1047

docs: Added tutorial topic #1047

Conversation

Doug-AWS commented Oct 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Also

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

rix0rrr commented Nov 1, 2018

Doug-AWS commented Nov 1, 2018

Doug-AWS commented Nov 1, 2018

rix0rrr commented Nov 2, 2018

Don't rebase!