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

Support Terraform 0.12.x configuration #113

Merged
merged 26 commits into from
Dec 20, 2019
Merged

Conversation

moatra
Copy link
Contributor

@moatra moatra commented Jun 17, 2019

This PR is meant as a proposal for adding Terraform 0.12 support. It replaces the current AST parsing with the hashicorp/terraform-config-inspect module.

(I picked up golang this weekend to make this change, so I apologize in advance for anything non-idiomatic. Feedback appreciated!)

Major changes:

  • Pulling descriptions from comments is no longer supported.
  • Dropped support for the --no-sort flag, as the terraform-config-inspect module doesn't seem to return blocks in source order
  • Dropped support for the --with-aggregate-type-defaults flag. All output for default is now json.
  • Markdown table output for variables is now done using html elements, to support multi line codeblocks for extended types and default values.
  • Added the --providers flag to include information on the provider configuration needed by the module.
  • Dropped the vendor/ directory
  • Changed path cli input from multiple directories or files to just one directory; (tfconfig.LoadModule() only supports loading one directory)

I updated master on the fork repo to directly support go get github.com/moatra/terraform-docs if you'd like to try it out locally.

Thoughts? Concerns? Changes?

(ref: #62 )

@metmajer
Copy link
Member

metmajer commented Jun 17, 2019

Hi @moatra, and thanks for your interest in this project! We know that support for Terraform 0.12 is important... and that it's a beast. Although I value your proposal, we can't simply throw away existing functionality from terraform-docs, such as the parsing of comments. I am well aware that this will require extensive integration with HCL 2 and the complexity of this migration and the fact that we're into full-time jobs doesn't allow for a quick approach.

I'll have a look at your implementation soon. Please understand though, that this migration has to be done carefully and feature complete and will have to be guided by more senior contributors.

@moatra
Copy link
Contributor Author

moatra commented Jun 17, 2019

Hey @metmajer! I completely understand; this PR is an experiment with using the terraform-config-inspect module to get 80% of the functionality for 20% of the work (and a project for me to learn go). I realize a lot of the changes in the PR are pretty aggressive, and if there's any future for the PR I'm happy to dial it back. I strongly doubt terraform-config-inspect will be willing to sniff comments, so if 100% feature support is mandatory, the approach in this PR won't work.

That said, my gut says that there's probably a good chunk of the terraform-docs user base that needs 0.12 support but not comment sniffing support. If so, is there a path forward with a transient release using terraform-config-inspect while the dev work for the correct solution is ongoing?

@cmeisinger
Copy link

My .02 is that I'd love a version which works with .12. Frankly terraform-docs w/o .12 support is an antiquated tool which makes me sad.

I've already converted a very large TF codebase to .12 and I'm sure I'm not alone.

Halp. :)

@sudoforge
Copy link

I've already converted a very large TF codebase to .12 and I'm sure I'm not alone.

Just to throw some additional weight behind this; I have somewhere around 200k lines of HCL that have been converted to HCL2 and previously relied upon terraform-docs for generating module documentation.

As I understand it, we are waiting on hashicorp/terraform-config-inspect#17 and hashicorp/terraform-config-inspect#18 -- is there any outstanding work we have to do outside of that to help support pushing an update that works with HCL2?

@metmajer
Copy link
Member

metmajer commented Aug 8, 2019

@sudoforge You may want to look at #62 for the current status. We're waiting for hashicorp/terraform-config-inspect contributors to elaborate on our feature and pull requests. You want to help? Here are a couple of suggestions:

  1. You don't have to convince us. We don't like the situation either. There is no easy migration path to HCL2, and I have decided to build on HashiCorp's own terraform-config-inspect project to provide for best backwards and forward compatibility. A custom integration would inevitably leave us one step behind. Like it or not.
  2. Push the maintainers at terraform-config-inspect to support our feature request. The project states that pull requests are not supported. Therefore, we need to ask for permission first.
  3. @antonbabenko mentions a workaround over at Add support for Terraform 0.12's Rich Value Types #62 which you could use for the time being.
  4. If you're relying heavily on terraform-docs and can live with the limitations provided by this pull request, you may want to continue with @moatra's contribution until we have a better solution available.
  5. Open source project maintainers don't get paid and run these projects in their spare time. If you think that our path is wrong, I invite you to suggest or provide a better solution.

@sudoforge
Copy link

@metmajer Woah, hey, it sounds like I may have misworded my earlier comment or that you may be misconstruing it; I was legitimately asking what was still outstanding so that I could help out (either via code or other means).

From one FOSS maintainer to another, many apologies.


I'll bug Hashicorp, and take a look at resolving the conflicts in this PR.

@moatra
Copy link
Contributor Author

moatra commented Aug 9, 2019

Here's a summary of where things stand today for generating Terraform 0.12 module docs:

@jstewmon
Copy link

jstewmon commented Aug 9, 2019

You could use terraform-config-inspect -json with gomplate to format however you like.

@metmajer
Copy link
Member

metmajer commented Aug 12, 2019

@jstewmon How would this be more helpful as opposed to using this PR? terraform-config-inspect lacks features that we depend on to remain backwards compatible.

@jstewmon
Copy link

@metmajer I was just offering a suggestion in direct response to this line in @moatra's comment:

You can use the terraform-config-inspect binary today to generate markdown for a 0.12 module. However, there's no way to change the format of the markdown. The hashicorp/terraform-config-inspect#17 PR will let you specify your own go template, which could make terraform-config-inspect a viable alternative to terraform-docs

@jakejx
Copy link

jakejx commented Oct 5, 2019

Hi! I am interested in contributing towards this PR. Are the primary blockers for this PR the lack of pulling description from comments and the lack of the --no-sort flag?

terraform-config-inspect exposes line numbers which should make it possible to implement --no-sort. Together with abit of manual parsing, it should be possible to extract out description comments too. Though it seems that @moatra's branch has diverged quite significantly from master so some manual merging might be required.

@metmajer
Copy link
Member

metmajer commented Oct 5, 2019

@njunxuan Thank you for offering your support! @khos2ow and I have decided to move on with this approach and sacrifice support for comments for the time being. And, you name it, support for --no-sort can be added by inspecting line numbers. @khos2ow let @njunxuan know if he can help in some way.

@jakejx
Copy link

jakejx commented Oct 6, 2019

should we keep 0.12 support behind a flag so that we do not break existing workflows?

@khos2ow
Copy link
Member

khos2ow commented Dec 13, 2019

@moatra I just created branch terraform-012 based off of the commit you had created this PR from. Would you be able to change the target of this PR that we can go ahead and merge it in terraform-012? Then we can start rebasing on master and fixing the conflicts from there.

Or alternatively you can enable Allow edits from maintainers on this PR for me to be able to continue the work on the same PR.

@khos2ow
Copy link
Member

khos2ow commented Dec 15, 2019

I'm in the middle of rebasing this PR and resolving the conflicts. I want to propose these changes while doing so:

  • remove --providers flag from here and add it in its own standalone PR for the sake of documentation and history
  • don't drop the support for --no-sort here because we've kinda figured out a way, and actually implement that in its own PR
  • don't drop the support for --with-aggregate-type-defaults altogether because it's a breaking change, rather than having it around (while internally doing nothing) and communicate the upcoming change and completely deprecate it in the following first or second release from now
  • don't rename from input to variable in here. I love the notion of deprecating input in favor of variables (which is a more natural Terraform wording) but it's gonna be a slight breaking change and doing so will introduce a lot of changes to existing markdown of the downstream users. May be we can come up with an opt-in support for it and eventually deprecate it in the following release or two

What I have in mind is to continue with the rebase and resolve all the conflicts and on the consecutive commits here apply the above changes I mentioned and finally proceed with this PR.

@moatra I'd love your feedback on this, and also from all the other folks here.

/cc @metmajer

@sudoforge
Copy link

@khos2ow I agree with the changes you have proposed; keeping PRs as small and focused as possible is a great goal.

@khos2ow
Copy link
Member

khos2ow commented Dec 20, 2019

I'm done with rebasing and fixing the issues. I've used the binary of this PR on some real world config of my own and some from opensource community, and the amount of different is minimal and acceptable. Although I suspect that's gonna grow because we're going to add provider and variable name in another PR, but still it'll be acceptable.

I'm gonna merge this PR now and continue on the points I mentioned earlier on consecutive PRs.

Thank you so much @moarta for the awesome initiative you took with the PR ❤️ 💯

Fixes #62
Fixes #131

@khos2ow khos2ow merged commit b8da8c5 into terraform-docs:master Dec 20, 2019
@khos2ow khos2ow mentioned this pull request Dec 20, 2019
1 task
@metmajer
Copy link
Member

Thanks @khos2ow for taking the lead on this one and making it happen!

khos2ow added a commit that referenced this pull request Dec 21, 2019
BREAKING CHANGE: - We've decided to rename `input` to `variable` which
is a known terminology in Terraform configuration. The affected parts
are: 1) markdown titles have been changed to `## Variables` 2) json item
has been renamed to `variables`.
- Also flag `--sort-inputs-by-required` has been deprecated in favor
of `--sort-by-required`. The depracted flag will be completely
removed in the second release from now.

Originally-Authored-By: Thomas Alton <thomas.alton@gmail.com>
Original-Pull-Request: #113
khos2ow added a commit that referenced this pull request Dec 21, 2019
BREAKING CHANGE: - We've decided to rename `input` to `variable` which
is a known terminology in Terraform configuration. The affected parts
are: 1) markdown titles have been changed to `## Variables` 2) json item
has been renamed to `variables`.
- Also flag `--sort-inputs-by-required` has been deprecated in favor
of `--sort-by-required`. The depracted flag will be completely
removed in the second release from now.

Originally-Authored-By: Thomas Alton <thomas.alton@gmail.com>
Original-Pull-Request: #113
ivanfetch added a commit to FairwindsOps/terraform-bastion that referenced this pull request Dec 29, 2019
… a Docker image for `terraform-docs`

The new GCP Terraform module manages a Google Cloud bastion, using Google DNS for the bastion host record, Stackdriver for logs, and Google Cloud Storage for SSH hostkeys and optional `additional external users`.

The AWS bastion module is moved to its own `aws` sub-directory - the next release of the AWS module will require users to update their `source` argument to include the `//aws` path and a module-specific release tag such as `aws-vx.y.z`.

A Docker image is used to incorparate `terraform-docs` output into per-module ReadMe files, until a version of terraform-docs is released that natively supports Terraform 0.12.
* Terraform-docs pull request: terraform-docs/terraform-docs#113
* Docker image and Terraform 0.12 workaround: cytopia/docker-terraform-docs#8
ivanfetch added a commit to FairwindsOps/terraform-bastion that referenced this pull request Dec 29, 2019
… a Docker image for `terraform-docs`

The new GCP Terraform module manages a Google Cloud bastion, using Google DNS for the bastion host record, Stackdriver for logs, and Google Cloud Storage for SSH hostkeys and optional `additional external users`.

The AWS bastion module is moved to its own `aws` sub-directory - the next release of the AWS module will require users to update their `source` argument to include the `//aws` path and a module-specific release tag such as `aws-vx.y.z`.

A Docker image is used to incorparate `terraform-docs` output into per-module ReadMe files, until a version of terraform-docs is released that natively supports Terraform 0.12.
* Terraform-docs pull request: terraform-docs/terraform-docs#113
* Docker image and Terraform 0.12 workaround: cytopia/docker-terraform-docs#8
@k-k
Copy link

k-k commented Dec 31, 2019

@khos2ow I'm excited to see this merged, what was the plan for tagging a release?

khos2ow added a commit to khos2ow/terraform-docs that referenced this pull request Jan 2, 2020
Originally-Authored-By: Thomas Alton <thomas.alton@gmail.com>
Original-Pull-Request: terraform-docs#113
@khos2ow khos2ow mentioned this pull request Jan 2, 2020
10 tasks
@khos2ow
Copy link
Member

khos2ow commented Jan 2, 2020

@kmfk I just came back from holiday break, I'm targeting to cut a RC release sometime next week.

khos2ow added a commit to khos2ow/terraform-docs that referenced this pull request Jan 3, 2020
Originally-Authored-By: Thomas Alton <thomas.alton@gmail.com>
Original-Pull-Request: terraform-docs#113
khos2ow added a commit that referenced this pull request Jan 3, 2020
* Show 'providers' information

* Fix for unsorted providers list

BREAKING CHANGE: - With Terraform 0.12 the information about `providers` being used in the module will be generated by default. This will cause the first generation of documents with the latest release of `terraform-docs` binary be slightly different than before, now there will be `Providers` section in Markdown and `providers` block in JSON. You can ignore this by using new `--no-providers` flag if you choose to.

Originally-Authored-By: Thomas Alton <thomas.alton@gmail.com>
Original-Pull-Request: #113
@khos2ow khos2ow added this to the v0.8.0 milestone Jan 6, 2020
ivanfetch added a commit to FairwindsOps/terraform-bastion that referenced this pull request Jan 31, 2020
… a Docker image for `terraform-docs` (#22)

* Add a GCP bastion module, move the AWS module to a sub-directory, use a Docker image for `terraform-docs`

The new GCP Terraform module manages a Google Cloud bastion, using Google DNS for the bastion host record, Stackdriver for logs, and Google Cloud Storage for SSH hostkeys and optional `additional external users`.

The AWS bastion module is moved to its own `aws` sub-directory - the next release of the AWS module will require users to update their `source` argument to include the `//aws` path and a module-specific release tag such as `aws-vx.y.z`.

A Docker image is used to incorparate `terraform-docs` output into per-module ReadMe files, until a version of terraform-docs is released that natively supports Terraform 0.12.
* Terraform-docs pull request: terraform-docs/terraform-docs#113
* Docker image and Terraform 0.12 workaround: cytopia/docker-terraform-docs#8

* Update GCP firewall rule to be optional, use the bastion name for the FW rule tag

The GCP firewall rule is optionally created, if the `ssh_cidr_blocks`
input is set to an empty list.

The GCP instance template and firewall rule use the bastion name as the
tag that associates the firewall rule with the bastion compute instance.

* Update GCP module providers to be inline with other related 3rd party modules

* GCP module additional external users GCS object is optional, other fixes

The additional external users script is only written to GCS if the
`additional_external_users` input is defined.

References to `user data` were renamed to `setup script` in template resources.

The Google provider version is further relaxed to 2.X.

* Update the GCP `ssh_public_key_file` input to behave like the AWS module

The `ssh_public_key_file` input was initially given an unfortunate name, and actually reflects the content
(not the file name) of an SSH public key. For now, the GCP module will
match the AWS one.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants