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

Allow use of 3 availability zones #23

Merged
merged 5 commits into from
Jul 28, 2019
Merged

Allow use of 3 availability zones #23

merged 5 commits into from
Jul 28, 2019

Conversation

fliphess
Copy link
Contributor

Allow use of 3 availability zones after hashicorp/terraform-provider-aws#9398

@aknysh
Copy link
Member

aknysh commented Jul 22, 2019

thanks @fliphess

Looks good, but please rebuild README by executing these commands:

make init
make readme/deps
make readme

It will add the new variables and outputs to README.md automatically.

In general, any changes to README should be made in README.yaml (not in this case), and after that executing the commands above will rebuild README.yaml into README.md and add all new variables and outputs to README.md

thanks

@aknysh
Copy link
Member

aknysh commented Jul 22, 2019

also please take a look at this terraform apply error (from the tests):

TestExamplesComplete 2019-07-22T13:38:33Z command.go:121: Error: Unsupported argument                                                                                                     
TestExamplesComplete 2019-07-22T13:38:33Z command.go:121:                                                                                                                                 
TestExamplesComplete 2019-07-22T13:38:33Z command.go:121:   on ../../main.tf line 129, in resource "aws_elasticsearch_domain" "default":                                                  
TestExamplesComplete 2019-07-22T13:38:33Z command.go:121:  129:       zone_awareness_enabled  = var.zone_awareness_enabled                                                                
TestExamplesComplete 2019-07-22T13:38:33Z command.go:121:                                                                                                                                 
TestExamplesComplete 2019-07-22T13:38:33Z command.go:121: An argument named "zone_awareness_enabled" is not expected here.                                                                
TestExamplesComplete 2019-07-22T13:38:33Z command.go:121:                                                                                                                                 
TestExamplesComplete 2019-07-22T13:38:33Z retry.go:80: Returning due to fatal error: FatalError{Underlying: exit status 1}                                                                

@fliphess
Copy link
Contributor Author

Ah thanks for pointing out where to look :)

I'll look into it, sorry for not looking after the tests and adjust correctly, I was at work and was hoping for a quick win :)

@fliphess fliphess changed the title Allow use of 3 availability zones WIP: Allow use of 3 availability zones Jul 22, 2019
@fliphess
Copy link
Contributor Author

fliphess commented Jul 22, 2019

@aknysh The tests look a lot better now, but still testing with 2 azs. What would you suggest for changing the tests?

I personally would prefer to test the module using 3 azs as well with 2, but that would I'm not in command of your infra and AWS billing so I'm not sure if I should just change the code example that is used for testing :)

@fliphess fliphess changed the title WIP: Allow use of 3 availability zones Allow use of 3 availability zones Jul 22, 2019
@aknysh
Copy link
Member

aknysh commented Jul 22, 2019

thanks @fliphess
feel free to add another AZ here https://github.com/cloudposse/terraform-aws-elasticsearch/blob/master/examples/complete/fixtures.us-west-1.tfvars#L9

@aknysh aknysh self-requested a review July 22, 2019 20:10
@aknysh
Copy link
Member

aknysh commented Jul 22, 2019

/codefresh run test

@fliphess
Copy link
Contributor Author

Did the last testrun succeed? I wasn't able to view the output......

@aknysh
Copy link
Member

aknysh commented Jul 24, 2019

/codefresh run test

@aknysh
Copy link
Member

aknysh commented Jul 24, 2019

@fliphess
we need to look into this in more details.
your changes are correct, but our tests use us-west-1 region that has only 2 AZs per account (and the AZs are different for diff accounts), so currently we can't test with 3 AZs in us-west-1

@osterman
Copy link
Member

/codefresh run test

2 similar comments
@aknysh
Copy link
Member

aknysh commented Jul 25, 2019

/codefresh run test

@aknysh
Copy link
Member

aknysh commented Jul 25, 2019

/codefresh run test

@fliphess
Copy link
Contributor Author

Hey @aknysh, Let me know what you have in mind, for now, I reverted the last commit that adds the third AZ so the test will succeed.

I think how to test in multiple availability zones is mostly a Cloudposse decision and not a a community effort as it's your billing and your AWS account :)

(Thanks btw for the high quality terraform modules, highly appreciated)

@fliphess
Copy link
Contributor Author

Maybe merge this PR and start a new issue to come up with a solution for the missing third availability zone? Keep me posted :)

@aknysh
Copy link
Member

aknysh commented Jul 28, 2019

/codefresh run test

1 similar comment
@aknysh
Copy link
Member

aknysh commented Jul 28, 2019

/codefresh run test

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 @fliphess
we'll update the tests to use 3 AZs in another PR

@aknysh aknysh merged commit 650d5c9 into cloudposse:master Jul 28, 2019
Shogan pushed a commit to Shogan/terraform-aws-elasticsearch that referenced this pull request Oct 3, 2019
* Allow use of 3 availability zones after hashicorp/terraform-provider-aws#9398

* Update module documentation

* Move zone_awareness out of zone_awareness_config

* Add an extra az to th tfvars

* Revert "Add an extra az to th tfvars"

This reverts commit cdc0d26.
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.

3 participants