Skip to content
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

Simplify built-in quickstarts to a simple hello world #849

Merged

Conversation

heitorlessa
Copy link
Contributor

@heitorlessa heitorlessa commented Dec 11, 2018

Issue #, if available:

Description of changes:

This PR simplifies all quickstarts turning them into a much simpler hello world that aims to make it easier for new customers to onboard.

Once complete, the initial experience will look like:

sam init

# Message output now displays next steps
[+] Initializing project structure...

Project generated: ./sam-app

Steps you can take next within the project folder
===================================================
[*] Invoke Function: sam local invoke HelloWorldFunction --event event.json
[*] Start API Gateway locally: sam local start-api

Read sam-app/README.md for further instructions

cd sam-app
sam local invoke HelloWorldFunction -e event.json

Checklist:

  • Simplify NodeJS quickstart
  • Simplify Python quickstart
  • Simplify Golang quickstart
  • Simplify Java quickstart
  • Simplify Dotnet Core quickstart
  • Simplify Ruby quickstart
  • Adjust AWS IAM permissions (PoweUser->Administrator) wording on all templates
  • Provide Commands to execute next message upon project initialization for strongly typed/dynamic runtimes
  • Add a Cleanup phase explaining how to delete an entire SAM Stack
  • Clean up Bringing to the next level
  • Comment code and tests that uses dependencies for Python
  • Comment code and tests that uses dependencies for Node
  • Comment code and tests that uses dependencies for Ruby
  • Add Learn how SAM Build can help you with dependencies section for those that support sam build
  • Update root template README to avoid confusion for contributors/users trying to clone instead of initializing a project
  • make pr passes
  • Update links pointing now pointing to official AWS Docs
  • Provide feedback on docs that are missing in the official AWS Docs and that cannot be updated in quickstarts (ie Globals, implicit generated resources)

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

│ └── test_handler.js
└── template.yaml <-- SAM template
│ └── app.js <-- Lambda function code
├── package.json <-- NodeJS dependencies and scripts
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

package.json must be within the function folder. Each function should get its own manifest - that's the best practice we heard from customers and we are codifying that within "sam build" as the default (obviously overridable)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right! Should we also move tests/ within the hello_world folder? If not, we'd also need to change npm run test behaviour.

Lemme know.


For context, under the Testing section we have:

npm install
npm run test

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ha! That's what happens when you have multiple copies of it locally. Resuming work on this, thanks @jfuss for pointing this out !!!!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I had to make another change so an user doesn't have to keep jumping in and out to run test, come back to where template.yaml is to run sam local invoke again, etc.

I included npm run start and npm run invoke scripts under hello_world/package.json for that matter.

Ideally, I think, for NodeJS, it'd be great to have a root package.json so one doesn't have to keep jumping to each source code folder and can do something like while still getting the benefit of having a per-function dependency:

  • npm run test:hello-world: This would take care of sam build && npm run test --prefix hello_world/
  • npm run invoke:hello-world: This would take care of sam local invoke HelloWorldFunction -e event.json

Which then can be duplicated as one creates more functions and ease the project to grow organically.

Source code tree as an example to visualize this:

.
├── README.md
├── event.json
├── hello_world
│   ├── app.js
│   ├── etc
│   ├── package.json                    <-- NodeJS dependencies and scripts
│   └── tests
│       └── unit
│           └── test_hello_world.js
├── package.json                        <-- NodeJS no deps w/ scripts for convenience
└── template.yaml

What are your thoughts? @jfuss @sanathkr

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@heitorlessa Any changes here are going to conflict with #845, where we are updating the nodejs examples to support sam build for Nodejs Lambdas.

My concern with having two package.json is: How do I know which one to use or update?

Copy link
Contributor Author

@heitorlessa heitorlessa Dec 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a totally valid concern @jfuss. After some thought and playing with --prefix I believe that will come down to preference at the end of the day.

Customers I worked with on Node with multiple functions in a repo they had this approach, others used a Makefile so no standard per se.

For this I will hold on any changes to this PR as we agree on what the SAM CLI Core team ultimately prefer as a quickstart

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@heitorlessa heitorlessa force-pushed the improv/simplify-init-quickstart-templates branch from 0622b9b to 7a82d9f Compare December 21, 2018 11:52
@heitorlessa
Copy link
Contributor Author

Completed rebase and pushed new NodeJS simplified along with a legacy template for Nodejs4.3/6.10 and dedicated instructions.

Working on updating the other templates after rebase

@heitorlessa
Copy link
Contributor Author

Description updated and will rebase and resume work after #866 is completed

@heitorlessa heitorlessa force-pushed the improv/simplify-init-quickstart-templates branch from 81e6125 to c6d5cdc Compare December 21, 2018 18:12
@heitorlessa heitorlessa changed the title [WIP] Simplify built-in quickstarts to a simple hello world Simplify built-in quickstarts to a simple hello world Dec 21, 2018
@heitorlessa
Copy link
Contributor Author

rebase complete and Ruby quickstart now follows other templates as requested.

Ready for review!

Copy link
Contributor

@jfuss jfuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please replicate the comments across the different init's. We should aim for them to all read consistently but deviate when needed for the language.

""".format(output_dir=output_dir, name=name)

no_build_step_required = (
"python", "python3.7", "python3.6", "python2.7", "nodejs", "nodejs4.3", "nodejs6.10", "nodejs8.10", "ruby2.5")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lambda has moved nodejs4.3 to depreciated, meaning creating and updating functions is no longer allows/supported. Can we match that expectation here and remove creation of Nodejs.4.3 functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, done!


no_build_step_required = (
"python", "python3.7", "python3.6", "python2.7", "nodejs", "nodejs4.3", "nodejs6.10", "nodejs8.10", "ruby2.5")
next_step_msg = no_build_msg if runtime in no_build_step_required else build_msg
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if a custom template takes in a the --runtime arg? This message may or may not be true then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what you mean by a custom template takes in 'a in the --runtime arg' -- This is a quick workaround until sam build fully supports all runtimes so the message will remain consistent and this will be deleted, though the workaround is flawed in the sense of as sam build supports more native runtimes this will need updating.

    no_build_step_required = (
        "python", "python3.7", "python3.6", "python2.7", "nodejs", "nodejs4.3", "nodejs6.10", "nodejs8.10", "ruby2.5")
    next_step_msg = no_build_msg if runtime in no_build_step_required else build_msg

--runtime arg is validated against the ones we support and no_build_step_required essentially borrows the ones that have sam build support.

What would you suggest instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I was getting two PRs mixed up, where I thought we were passing runtime to cookiecutter examples that are passed to --location. You can ignore this comment and can handle in the other pr that enables that.

"nodejs8.10": os.path.join(_templates, "cookiecutter-aws-sam-hello-nodejs"),
"nodejs4.3": os.path.join(_templates, "cookiecutter-aws-sam-hello-nodejs"),
"nodejs4.3": os.path.join(_templates, "cookiecutter-aws-sam-hello-nodejs-legacy"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove 4.3 support since Lambda does not support creating or updating functions with runtimes of nodejs4.3

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


Create your own .NET Core solution template to use with SAM CLI. [Cookiecutter for AWS SAM and .NET](https://github.com/aws-samples/cookiecutter-aws-sam-dotnet) provides you with a sample implementation how to use cookiecutter templating library to standardise how you initialise your Serverless projects.
Next, you can use AWS Serverless Application Repository to deploy ready to use Apps that go beyond hello world samples and learn how authors developed their applications: [AWS Serverless Application Repository main page](https://aws.amazon.com/serverless/serverlessrepo/)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could call out the sam publish command here.

--query 'Stacks[].Outputs[?OutputKey==`HelloWorldApi`]' \
--output table

# Tail Lambda function Logs using Logical name defined in SAM Template
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have a step to invoke the function or API gateway locally with sam logs running in the terminal?

@@ -12,7 +12,7 @@ Globals:
Resources:

HelloWorldFunction:
Type: AWS::Serverless::Function # More info about Function Resource: https://github.com/awslabs/serverless-application-model/blob/master/versions/2016-10-31.md#awsserverlessfunction
Type: AWS::Serverless::Function # More info about Function Resource: https://docs.aws.amazon.com/serverless-application-model/latest/developerguide/serverless-sam-template.html#serverless-sam-template-function
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should keep the GH links here. The Spec definitions are held on GH.

* `pipenv install cookiecutter`

**NOTE**: [`Pipenv`](https://github.com/pypa/pipenv) is the new and recommended Python packaging tool that works across multiple platforms and makes Windows a first-class citizen.
* [AWS SAM CLI](https://github.com/awslabs/aws-sam-cli)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this point to our landing page instead: https://aws.amazon.com/serverless/sam/


You can issue the following command in a shell to build it:
**Building an executable target for Lambda runtime**

```shell
GOOS=linux GOARCH=amd64 go build -o hello-world/hello-world ./hello-world
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this just use the Makefile instead?


#### Chocolatey (Windows)
### Step-through debugging
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be this section in the .Net as well

```

## Fetch, tail, and filter Lambda function logs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be in the .Net example as well.

@heitorlessa heitorlessa force-pushed the improv/simplify-init-quickstart-templates branch from 4bff4ab to 8340bb5 Compare January 7, 2019 19:36
Copy link
Contributor Author

@heitorlessa heitorlessa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed Jacob's feedback and clarified next_step_msg workaround

""".format(output_dir=output_dir, name=name)

no_build_step_required = (
"python", "python3.7", "python3.6", "python2.7", "nodejs", "nodejs4.3", "nodejs6.10", "nodejs8.10", "ruby2.5")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, done!

"nodejs8.10": os.path.join(_templates, "cookiecutter-aws-sam-hello-nodejs"),
"nodejs4.3": os.path.join(_templates, "cookiecutter-aws-sam-hello-nodejs"),
"nodejs4.3": os.path.join(_templates, "cookiecutter-aws-sam-hello-nodejs-legacy"),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


no_build_step_required = (
"python", "python3.7", "python3.6", "python2.7", "nodejs", "nodejs4.3", "nodejs6.10", "nodejs8.10", "ruby2.5")
next_step_msg = no_build_msg if runtime in no_build_step_required else build_msg
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what you mean by a custom template takes in 'a in the --runtime arg' -- This is a quick workaround until sam build fully supports all runtimes so the message will remain consistent and this will be deleted, though the workaround is flawed in the sense of as sam build supports more native runtimes this will need updating.

    no_build_step_required = (
        "python", "python3.7", "python3.6", "python2.7", "nodejs", "nodejs4.3", "nodejs6.10", "nodejs8.10", "ruby2.5")
    next_step_msg = no_build_msg if runtime in no_build_step_required else build_msg

--runtime arg is validated against the ones we support and no_build_step_required essentially borrows the ones that have sam build support.

What would you suggest instead?

Copy link
Contributor

@jfuss jfuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WOOT! I know it took some time to get it through, so thanks for keeping with it.

@jfuss jfuss dismissed sanathkr’s stale review January 14, 2019 16:16

jfuss approved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants