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

Enhance README to take advantage of terraform.tf #46

Merged
merged 9 commits into from
Jun 24, 2020
Merged

Enhance README to take advantage of terraform.tf #46

merged 9 commits into from
Jun 24, 2020

Conversation

jmcgeheeiv
Copy link
Contributor

what

  • Add terraform.tf to the usage instructions

why

  • The current usage does not account for terraform.tf, which is actually quite brilliant functionality

references

  • Personal experience using this over the last few months

@jmcgeheeiv jmcgeheeiv requested a review from a team as a code owner May 18, 2020 02:43
@jmcgeheeiv
Copy link
Contributor Author

I added a destroy procedure. Using this module this way works very smoothly. Very nice architecture!

@schollii
Copy link
Contributor

I was just thinking the other day of doing something like this, I didn't know such functionality was already in place just not documented!

However I'm wondering since it is so far undocumented can we make the following changes before this goes "public":

  • rename those 3 input variables, they are unnecessarily verbose and clash with rest. Given the context of this module, it would be sufficient to call them just backend_file_path etc.
  • the path to the generated file should probably default to root module path, ie almost always the module that ultimately needs a backend. So the count 0/1 test should be whether the filename has been defined, not whether the path has been defined.
  • I think you have to empty bucket after state moved locally, before terraform destroy, otherwise you get an error.

I would be happy to submit some PR for same changes in other repos if that helps, I don't remember seeing any of them advertise this approach either.

@osterman
Copy link
Member

This is an interesting approach!

@osterman osterman requested a review from 3h4x May 21, 2020 21:34
@schollii
Copy link
Contributor

pretty sure you will have to set force_destroy to true before doing the final apply since the bucket is versioned

@jmcgeheeiv
Copy link
Contributor Author

Yes, I encountered that last time I did terraform destroy. I'm also reconsidering whether the backend file should be added to .gitignore, or whether it should be committed to Git. I want to try the workflow a few more times, but right now I'm away from this part of the project now. Still, the basic workflow is quite good. It is CloudPosse that deserves credit for that, @osterman.

@osterman
Copy link
Member

Sorry we haven’t been able to review this yet. We’ve been slammed with new projects.

Has anyone else had a chance to test this out?

@jmcgeheeiv
Copy link
Contributor Author

No rush, @osterman. In fact, I will change this to WIP because I still want to check into the issues I mentioned in my last comment.

@jmcgeheeiv jmcgeheeiv changed the title Enhance README to take advantage of terraform.tf WIP: Enhance README to take advantage of terraform.tf May 29, 2020
@schollii
Copy link
Contributor

schollii commented May 30, 2020

@jmcgeheeiv even though the file is auto-generated I think it should be committed: if there is a change in the tf state module, or you make a change in input values, that you didn't realize would affect it, you will be able to see the change with git diff. Whereas without having committed it, you will only see what the plan tells you has changed, there will be no source to check for diff to help understand cause of change.

Also is there any disadvantage? If a timestamp or other attribute in the generated file changed at every apply, that would be a good reason to gitignore it, but that's not the case.

@jmcgeheeiv
Copy link
Contributor Author

@schollii, in my most recent pass through the workflow I have made these changes:

  1. Specify file name ./backend.tf. terraform.tf is not descriptive.
  2. Commit ./backend.tf to git
  3. Do something more forceful to remove the state bucket. @schollii, could you help by describing the exact command you recommend?

Shall I implement these changes in the documentation?

@schollii
Copy link
Contributor

schollii commented May 30, 2020

I gather this will work on v0.16.0? Pretty sure I had issues with 0.17.0 (#47). It seems like it's only the docs that are changing. I believe the workflow is this:

  1. create backend-s3-bootstrap.tf
  2. terraform init
  3. terraform apply: creates backend.tf and local state
  4. terraform init: moves local state to s3
  5. terraform apply: creates other state if added, so this is optional if no additional resources created
  6. ...
  7. remove backend.tf
  8. terraform init: copy state from s3 to local
  9. edit bucket to have force_destroy=true
  10. terraform apply: updates only bucket; I'm not 100% this is necessary
  11. terraform destroy

However I cannot verify that right now, but if you don't do that, then during terraform destroy you will get an error that the bucket has contents. After I deleted manually from AWS console a couple weeks ago, the destroy worked.

@schollii
Copy link
Contributor

schollii commented Jun 9, 2020

@jmcgeheeiv I have verified the workflow and documented at #44 (comment) since PR's are harder to find. HTH.

@Gowiem
Copy link
Member

Gowiem commented Jun 18, 2020

@jmcgeheeiv Definitely like the idea of getting this merged. I personally didn't know about this functionality and it sounds much preferred! Weighing in on a couple things:

  1. backend.tf > terraform.tf 👍
  2. Committing backend.tf to git -- I only see negatives in not doing this. I think this is the right call.

Happy to get this PR approved, merged, and cut for you as soon as you and @schollii suss out the final changes you want to make to the README. Thanks gents!

@Gowiem Gowiem requested review from Gowiem and removed request for a team June 18, 2020 19:36
schollii
schollii previously approved these changes Jun 19, 2020
Copy link
Contributor

@schollii schollii left a comment

Choose a reason for hiding this comment

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

Looks good to me. FWIW I use this in my backend-aws.tf (the file that uses the tfstate_backend module):

  terraform_backend_config_file_name = "backend.tf"
  terraform_backend_config_file_path = var.tf_state_is_remote ? "." : ""
  force_destroy                      = ! var.tf_state_is_remote

then in my settings.auto.tfvars I just define tf_state_is_remote to be true or false. You just have to remember when you toggle between true and false to do a terraform init. Then the apply will actually remove or create the backend.tf, pretty cool. But I think this is too complicated to explain in the readme.

To be honest Hashicorp should do this automatically, ie it should be possible to tell the s3 backend block what bucket etc to create and it should create and use it, so we wouldn't have to go through this rigmarole.

@jmcgeheeiv
Copy link
Contributor Author

jmcgeheeiv commented Jun 19, 2020

Modified to match @schollii's procedure in #44 without question--because he seems like such a trustworthy chap.

@schollii, @Gowiem, please scrutinize.

Gowiem
Gowiem previously approved these changes Jun 19, 2020
Copy link
Member

@Gowiem Gowiem left a comment

Choose a reason for hiding this comment

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

@jmcgeheeiv I think that looks great! 👍

Since this is a collab with @schollii, I'll let him weigh in too before merging.

Thanks gents!

@jmcgeheeiv jmcgeheeiv changed the title WIP: Enhance README to take advantage of terraform.tf Enhance README to take advantage of terraform.tf Jun 19, 2020
Today I had an opportunity to destroy a deployment, and
I found an error in my destroy proceedure.

I also added @schollii as a contributor.
Today I had an opportunity to destroy a deployment, and
I found an error in my destroy proceedure.

I also added @schollii as a contributor.
@jmcgeheeiv
Copy link
Contributor Author

jmcgeheeiv commented Jun 19, 2020

Today I had an opportunity to destroy a deployment, and I found an error in my destroy procedure (wrong value for force_destroy).

I also added @schollii as a contributor. I hope you do not mind. If this bothers you, I can remove you from my fork and nobody will be the wiser.

@osterman
Copy link
Member

/rebuild-readme

@Gowiem
Copy link
Member

Gowiem commented Jun 23, 2020

@jmcgeheeiv Sorry to do it to ya, but it looks like there are some merge conflicts. Mind fixing and we'll get this merged?

@jmcgeheeiv jmcgeheeiv requested a review from a team as a code owner June 24, 2020 03:22
definition file. Note that when `terraform_backend_config_file_path` is
empty (the default), no file is created.

1. `terraform init`. This downloads Terraform modules and providers.
Copy link
Member

Choose a reason for hiding this comment

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

why all steps are numbered 1? :)
I see it was like that before, but maybe now is a good time to fix that.

force_destroy = true
}
```
1. `terraform apply -target module.terraform_state_backend -auto-approve`.
Copy link
Member

Choose a reason for hiding this comment

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

all steps are numbered as 1

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 @jmcgeheeiv
please see comments

@schollii
Copy link
Contributor

schollii commented Jun 24, 2020

@aknysh markdown doesn't require sequence of numbers just first one must be '1'. See https://www.markdownguide.org/basic-syntax/#ordered-lists.

However the readme is understandable without rendering if we use sequence of numbers. For small lists it's manageable, so I'm in favour of fixing.

@RothAndrew
Copy link
Sponsor Contributor

RothAndrew commented Jun 24, 2020

IMO 1s are preferable, since you dont need to reorder them if you add an item in the middle of the list

@aknysh
Copy link
Member

aknysh commented Jun 24, 2020

IMO 1s are preferable, since you dont need to reorder them if you add an item in the middle of the list

ah yes, the user will see it as an unordered list, it's just in the code itself we have 1.

@RothAndrew
Copy link
Sponsor Contributor

RothAndrew commented Jun 24, 2020 via email

@aknysh
Copy link
Member

aknysh commented Jun 24, 2020

They will see it as an ordered list

@aknysh aknysh closed this Jun 24, 2020
@Gowiem
Copy link
Member

Gowiem commented Jun 24, 2020

@aknysh Did you mean to close this? I would think we would still want to get this merged. Mind if I reopen?

@aknysh
Copy link
Member

aknysh commented Jun 24, 2020

@aknysh Did you mean to close this? I would think we would still want to get this merged. Mind if I reopen?

oh sorry, clicked the wrong button, reopening

@aknysh aknysh reopened this Jun 24, 2020
@jmcgeheeiv
Copy link
Contributor Author

@Gowiem
Copy link
Member

Gowiem commented Jun 24, 2020

/test all

Copy link
Member

@Gowiem Gowiem left a comment

Choose a reason for hiding this comment

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

I think this looks good and we've hashed it out plently. Let's ship it! :shipit:

@Gowiem Gowiem merged commit a367b5b into cloudposse:master Jun 24, 2020
@Gowiem
Copy link
Member

Gowiem commented Jun 24, 2020

@jmcgeheeiv @schollii Thanks for the contributions and wading through the updates! This is just a docs update, but still... Released as 0.18.1.

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.

None yet

7 participants