-
Notifications
You must be signed in to change notification settings - Fork 422
bunch of updates #139
base: master
Are you sure you want to change the base?
bunch of updates #139
Conversation
- latest xenial ubuntu release base - amazon/amazon-ecs-agent docker image uses the latest tag now - segment/ecs-logs from 0.1.1 to 0.1.5 - ixgbevf from 3.1.2 to 3.4.3 - terraform 0.7.2 to 0.9.11 - packer 0.10.1 to 1.0.3 - dumb-init from 1.1.3 to 1.2.0 - some bash syntax best practices here or there.
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.
Those are great changes, thanks a lot for your contributions!
I left a couple of comments inline, once addressed I'll be happy to merge this.
@@ -23,6 +23,7 @@ variable "cidr" { | |||
} | |||
|
|||
variable "default_ecs_ami" { | |||
type = "map" |
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.
Do you mind running terraform fmt
, I think the formatting is incorrect 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.
done!
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.
Also please run a final check beforehand, just to make sure.
# https://www.packer.io/docs/builders/amazon-ebs.html | ||
ami: | ||
source_ami: ami-e6d5d2f1 | ||
source_ami: ami-d15a75c7 |
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.
Is this the ID of an official image? Do you mind giving a link to it so we can verify how it was built?
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.
ofc.
you can find it here: https://cloud-images.ubuntu.com/locator/ec2/
also:
https://console.aws.amazon.com/ec2/v2/home?region=us-east-1#Images:visibility=public-images;search=ami-d15a75c7;sort=desc:name
same 099720109477 origin AWS account id.
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.
If your making updates to leverage handy new terraform features, why not throw in the AMI data resource? https://www.terraform.io/docs/providers/aws/d/ami.html
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 in the next one? Although Im wary of unrepeatable builds. You could.argue that I have sent the ECS agent to the latest docker tag. Your call.
EDIT: @gjohnson did you mean at the ecs-cluster module level aiming for "^myami-\d{3}" of the self account ?
Sorry for bothering you @achille-roussel, Is there a gitter/slack/IRC room we can discuss the stack so I can avoid hijacking the pull requests? |
Can this be merged? |
Would be really nice if this could be merged :) |
I assume this repository has largely been abandoned and also ecs has changed significantly in the last year. I've used Segment's stack as guide and built a new stack module from scratch with the following key differences:
If it is of interest to anyone, I'd be happy to publish it publicly. |
@raja That would be really cool, thanks. In my local version, I have also created a |
@raja and @martinkoch-geniebelt I've come to the similar conclusion that I should just use this repo as a template (which is a great start!) and modify to suit my own needs, but it would still be great to keep "the template" up to date somewhere. Please publish your fork if you're able to. @raja and @martinkoch-geniebelt -- @slajax also has added ALB support in PR #134 |
Is there a reason this hasn't been merged? |
Most large PRs don’t end up merging here because doing so would impact all the downstream environments the next time a source is pulled. The approach it seems many of us are using is to maintain a fork that you can point all your sources to and just PR to this repo to share with others - If they want the change they can add your fork as a remote and diff / patch as needed. We’re maintaining our fork actively and have rolled out ALB support on internal services as well for example. Anyone who wants this change is welcome to it but ultimately it would be risky for the segment team to merge it here. It would require a lot of community engagement to do these changes right since terraform doesn’t yet have a robust way to deal with semantic versioning of external modules - just refs, which is insufficient for managing the roll out of breaking changes in a stable way. No one wants downtime because of upstream infra changes. Tread carefully. |
@slajax Is your fork public? |
Yes. See open pull requests. |
@@ -27,7 +27,7 @@ ExecStart=/usr/bin/docker run \ | |||
--publish=127.0.0.1:51679:51679 \ | |||
--env-file=/etc/ecs/ecs.config \ | |||
--env=ECS_CLUSTER=${SERVER_GROUP} \ | |||
amazon/amazon-ecs-agent:v1.14.1 | |||
amazon/amazon-ecs-agent:latest |
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.
Dont know if this is a great idea. It happened to me twice that they introduce a critical bug in the latest version and screwed up all my cluster. Personally I would prefer to have that stuck to a fixed version
so far: