Skip to content

Conversation

@goruha
Copy link
Member

@goruha goruha commented Jun 28, 2018

What

  • ECR for kops

Why

  • Deploy Docker images to K8S from ECR

@goruha goruha requested a review from osterman June 28, 2018 20:41
source = "git::https://github.com/cloudposse/terraform-aws-kops-ecr.git?ref=tags/0.1.0"
namespace = "${var.namespace}"
stage = "${var.stage}"
name = "langing"
Copy link
Member

Choose a reason for hiding this comment

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

Typo

Copy link
Member

Choose a reason for hiding this comment

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

make it a variable. Add terraform.tfvars.example with defaults.

sarkis
sarkis previously approved these changes Jun 29, 2018
source = "git::https://github.com/cloudposse/terraform-aws-kops-ecr.git?ref=tags/0.1.0"
namespace = "${var.namespace}"
stage = "${var.stage}"
name = "${element(repositories_names,0)}"
Copy link
Member

Choose a reason for hiding this comment

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

This is an invalid variable :P

Copy link
Member

Choose a reason for hiding this comment

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

Also, I don't understand why it's valid that we create an ECR for only the first (0) repository.

aws/ecr/main.tf Outdated
}

variable "repositories_names" {
type = "list"
Copy link
Member

Choose a reason for hiding this comment

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

Seems like this should be a string or our ecr module should be made to support lists.

Copy link
Member

@osterman osterman left a comment

Choose a reason for hiding this comment

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

See inline comments

@goruha goruha merged commit ce857e6 into master Jul 2, 2018
@goruha goruha deleted the feature-add-ecr branch July 2, 2018 17:24
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.

4 participants