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

Fix count logic for ebs, make variable region non-required #5

Merged
merged 3 commits into from
Jan 25, 2019

Conversation

SweetOps
Copy link
Contributor

@SweetOps SweetOps commented Jan 15, 2019

what

  • fix count logic for ebs
  • make variable region non-required

@osterman osterman requested review from osterman and aknysh January 16, 2019 22:04
@osterman
Copy link
Member

Hey old friend! We’ll get this reviewed shortly.

main.tf Outdated
@@ -90,7 +92,8 @@ resource "aws_instance" "default" {
delete_on_termination = "${var.delete_on_termination}"
}

tags = "${merge(module.label.tags, map("instance_index", "${count.index}"))}"
tags = "${merge(module.label.tags, map("instance_index", "${count.index}"))}"
volume_tags = "${merge(module.label.tags, map("instance_index", "${count.index}"))}"
Copy link
Member

@aknysh aknysh Jan 16, 2019

Choose a reason for hiding this comment

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

looks like volume_tags is not in use.
and it's the same as tags?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, isn't a same. ltmgfy :)

volume_tags - (Optional) A mapping of tags to assign to the devices created by the instance at launch time.

in case of module it's rood device i.e. /

Copy link
Member

Choose a reason for hiding this comment

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

@SweetOps
they are the same in the code, and volume_tags is not in use anywhere.
please review it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tbh, I don't understand what you want to hear. Why the both of them have same tags? It is necessary for consistency e.g. instance with name cp-test-kafka-1 should has the same name for root device (cp-test-kafka-1) and same tags as instance to which it assigned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

volume_tags has been removed due hashicorp/terraform#3531 (comment)

aknysh
aknysh previously requested changes Jan 16, 2019
Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

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

thanks @SweetOps
please see comments.

Also, don't modify README.md, make all changes in README.yaml and then
rebuild README by executing these three commands:

make init
make readme/deps
make readme

@SweetOps SweetOps changed the title Fix count logic for ebs, add tags for root device, make variable region non-required Fix count logic for ebs, make variable region non-required Jan 18, 2019
@SweetOps
Copy link
Contributor Author

@osterman @aknysh , is there any chance to merge this PR? :)

osterman
osterman previously approved these changes Jan 25, 2019
@osterman osterman dismissed stale reviews from aknysh and themself January 25, 2019 09:11

Resolved

@osterman osterman merged commit 368e156 into cloudposse:master Jan 25, 2019
@hfarukuslu
Copy link

I do not understand what this has to do with "hashicorp/terraform#3531",
#3531 is a complete different problem and because of this merge #3531 was closed.

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