-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feat(docs): Updated examples topic to use ECS L2 construct #1195
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should ultimately also add a load balancer to make it do something useful I think.
docs/src/examples.rst
Outdated
|
||
Creating an App with Multiple Stacks | ||
==================================== | ||
Creating an |ECS| Construct |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't call it "creating an ECS construct", that implies you're building a reusable thing.
Maybe "Writing an |ECS| CDK app" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point.
docs/src/examples.rst
Outdated
Implement the class **MyStack** in the *my-stack* sub-folder, | ||
that extends the |stack-class| class | ||
(this is the same code as shown in the :doc:`concepts` topic). | ||
* Automatic security group opening for load balancers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are not sentences, maybe rephrase to a list of the form "it will open up ..." etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. I wrote these based on some notes and forgot to re-visit.
docs/src/examples.rst
Outdated
|
||
import { Stack, StackProps } from '@aws-cdk/cdk' | ||
Step 1: Create the Directory and Initialze the |cdk| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo. Wouldn't capitalize Directory and Initialize here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is AWS docs style. We capitalize pretty much everything but articles (the, and, etc.).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still typo
docs/src/examples.rst
Outdated
import ec2 = require('@aws-cdk/aws-ec2'); | ||
import rds = require('@aws-cdk/aws-rds'); | ||
import cdk = require('@aws-cdk/cdk'); | ||
There are two different ways of creating a serverless infrastructure with |ECS|: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would write "two different ways of running your container tasks"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
docs/src/examples.rst
Outdated
const vpc = new ec2.VpcNetwork(this, 'VPC'); | ||
This example creates a Fargate service, | ||
which requires a VPC, a cluster, a task definition, and a security group. | ||
Later on we'll show you how to launch an EC2 service. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how to launch services on EC2 instances you manage yourself
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
docs/src/examples.rst
Outdated
} | ||
} | ||
// The task definition must have at least one container. | ||
// Otherwise you cannot synthesize or deploy your app. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds like a constraint if you put it like this. But also, if you don't add a container, WHAT are you even launching? Empty tasks that do nothing? That's not very useful.
So I wouldn't call attention to it in this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rewrote the first comment line and removed the second comment line.
docs/src/examples.rst
Outdated
// The task definition must have at least one container. | ||
// Otherwise you cannot synthesize or deploy your app. | ||
taskDefinition.addContainer('MyContainer', { | ||
image: ecs.ContainerImage.fromDockerHub('MyDockerImage') // Required |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe launch amazon/amazon-ecs-sample
, that's a container that at least does something.
docs/src/examples.rst
Outdated
taskDefinition: taskDefinition, // Required | ||
cluster: cluster, // Required | ||
desiredCount: 6, // Default is 1 | ||
securityGroup: securityGroup, // Required |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it's not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep
docs/src/examples.rst
Outdated
|
||
const app = new cdk.App(); | ||
const securityGroup = new ec2.SecurityGroup(this, 'MySecurityGroup', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually not necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
docs/src/examples.rst
Outdated
cluster: cluster, // Required | ||
desiredCount: 6, // Default is 1 | ||
securityGroup: securityGroup, // Required | ||
platformVersion: ecs.FargatePlatformVersion.Latest // Default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't even call attention to this, just leave it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
I've updated the list of benefits and am deploying the code now to make sure it works (50 minutes and counting): 28/30 | 11:50:59 | CREATE_COMPLETE | AWS::EC2::Route | MyVpc/PrivateSubnet2/DefaultRoute (MyVpcPrivateSubnet2DefaultRoute9CE96294) |
Strange. I nuked my deployment and the CFN stack. then started all over with a fresh project. The code deployed in about 3 1/2 minutes. The template grew from about 300 lines to almost 500. |
docs/src/examples.rst
Outdated
|
||
import { Stack, StackProps } from '@aws-cdk/cdk' | ||
Step 1: Create the Directory and Initialze the |cdk| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still typo
docs/src/examples.rst
Outdated
|
||
.. _dynamodb_example: | ||
Update *my_ecs_construct.ts* in the *bin* directory to only contain the following code: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will have gotten changed to lib/my_ecs_construct_stack.ts
by the time this goes out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed the typo (sheesh); I just tested cdk init --language typescript
in 0.18.1 and it still creates the TS file in the bin folder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know. The change is not released yet. That's why I wrote:
by the time this goes out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this part as the template has changed.
docs/src/examples.rst
Outdated
.. code-block:: ts | ||
|
||
// Add capacity to it | ||
cluster.addDefaultAutoScalingGroupCapacity({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, no. This is to add EC2 capacity to run Ec2Services on. You've been creating a FargateService so you don't need to do this.
You want what this section says:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll take it out for now.
docs/src/examples.rst
Outdated
|
||
.. _dynamodb_example: | ||
Update *my_ecs_construct.ts* in the *bin* directory to only contain the following code: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know. The change is not released yet. That's why I wrote:
by the time this goes out.
docs/src/examples.rst
Outdated
readCapacity: 5, | ||
writeCapacity: 5 | ||
}); | ||
const app = new cdk.App(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact these lines will also be gone.
These lines will live in a file in the bin
directory, the other lines will live in a file in the lib
directory.
Here's the new version of the app template: https://github.com/awslabs/aws-cdk/tree/master/packages/aws-cdk/lib/init-templates/app/typescript
|CFN| displays information about the dozens of steps that | ||
it takes as it deploys your app. | ||
|
||
That's how easy it is to create a Fargate service to run a Docker image. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's true. The example is not yet useful without a load balancer though, since this is a web service but you will be unable to hit the endpoints (because they'll be in a private VPC if I'm not mistaken).
Just sayin'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you planning to make this section longer, or is it the plan to publish like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add a load balancer and auto-scaling for tasks to the example. Once the new template is released, I'll fix the references:
my_ecs_construct.ts in the bin directory -> my_ecs_stack.ts in the lib directory
docs/src/examples.rst
Outdated
}); | ||
|
||
// Add capacity to it | ||
cluster.addDefaultAutoScalingGroupCapacity({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need capacity if you're starting a Fargate task
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. I saw your comment to Paul in issue #1279.
I'm still blocked from confirming cdk deploy by issue #1279 |
docs/src/examples.rst
Outdated
@@ -65,7 +67,7 @@ gives you a leg up on using AWS services by providing the following benefits: | |||
* Automatically adds permissions for |ECR| if you use an image from |ECR| | |||
When you use an image from |ECR|, the |cdk| adds the correct permissions. | |||
|
|||
* Convenient API for autoscaling | |||
* Automatic autoscaling |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this mean? It's not automatic yet. I think we may have miscommunicated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, so what should it say? Should I go back to:
- Convenient API for autoscaling
The |cdk| supplies a method so you can autoscale instances when you use an |EC2| cluster;
this functionality is done automatically when you use an instance in a Fargate cluster.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.