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

Changed SimpleECSService to use LogGroup #15

Open
wants to merge 1 commit into
base: add_ecs_blueprint
Choose a base branch
from
Open

Changed SimpleECSService to use LogGroup #15

wants to merge 1 commit into from

Conversation

jeffreybian
Copy link

Now SimpleECSService and SimpleFargateService are consistently
using LogGroup instead of LogConfiguration, making it really simple.

For other drivers like syslogd, user can subclass the SimpleECSService.

Now SimpleECSService and SimpleFargateService are consistently
using LogGroup instead of LogConfiguration, making it really simple.
@jeffreybian
Copy link
Author

The awslogs driver will work with ecs-agent version > 1.9.0, which should be fine in most cases.

Copy link
Member

@phobologic phobologic left a comment

Choose a reason for hiding this comment

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

Mostly a small nit and then you should remove the task execution stuff, otherwise the log config stuff looks good. We should probably, in the long run, make it more configurable (maybe allow a log configuration, and if that's not given use the LogGroup setting to automatically use a Cloudwatch logs LogGroup with the awslogs) but this is good for now!

@@ -398,6 +418,46 @@ def create_role(self):
self.add_output("RoleArn", self.role.GetAtt("Arn"))
self.add_output("RoleId", self.role.GetAtt("RoleId"))

def create_task_execution_role(self):
Copy link
Member

Choose a reason for hiding this comment

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

So this actually won't work in the EC2 version of ECS. TaskExecutionRoles are meant to be used with Fargate from my understanding, and with EC2 the effective task execution role is the one that is applied to your Container Instance's (EC2) InstanceProfile.

So basically we just need to include the policy statements that you would normally include here in the Instance role/policy (which isn't something we can do here).

Copy link
Member

Choose a reason for hiding this comment

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

@@ -411,7 +471,7 @@ def create_policy(self):
self.policy = t.add_resource(
iam.ManagedPolicy(
"ManagedPolicy",
PolicyDocument=self.generate_policy(),
PolicyDocument=policy_doc,
Copy link
Member

Choose a reason for hiding this comment

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

Good catch!

log_config = self.get_variables()["LogConfiguration"]
return log_config or NoValue
"""
If LogConfiguration is given, go with it. Otherwise if LogGroup is given,
Copy link
Member

Choose a reason for hiding this comment

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

nit: This comment isn't quite right - we don't let LogConfiguration be given any longer.

@jeffreybian
Copy link
Author

jeffreybian commented Jun 27, 2018

Thank you! will make the changes. I will look more into the AWS documentation.

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.

2 participants