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

DOC: Update terraform_state.py #152

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lingfish
Copy link

SUMMARY

Update the description to be more thorough about what this inventory plugin does.

ISSUE TYPE
  • Docs Pull Request
COMPONENT NAME

terraform_state.py

ADDITIONAL INFORMATION

As a newcomer to this plugin, I saw odd results that I wasn't expecting, based on the documentation provided. I was expecting it to work with the bpg/proxmox Terraform provider for example, which it doesn't.

Update the description to be more thorough about what this inventory plugin does.
Copy link

Comment on lines +13 to +14
- This plugin works by making a temporary directory, writing a C(main.tf) file with the configured backend, then running C(terraform init),
followed by C(terraform show) in JSON format to read state.
Copy link
Member

Choose a reason for hiding this comment

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

These are implementation details and not something I think should appear in the module docs. There's currently a PR (#153) that changes this.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I obviously disagree. The current way it works should be stated somewhere (perhaps in NOTES?) I think considering the potential for danger in running something like init, it should be quite clear what it currently does. It currently states "terraform state is used", so I felt it needed fleshing out.

There's currently a PR (#153) that changes this.

If the PR changes this, then the doco can change at that point?

The parameters section documents things that implement changes, so 🤷

followed by C(terraform show) in JSON format to read state.
- The plugin accepts a Terraform backend config to an existing backend, or path(s) to files containing backend config.
- Uses a YAML configuration file that ends with C(terraform_state.yml) or C(terraform_state.yaml). For example, C(aws_terraform_state.yaml).
- Currently only supports the following Terraform providers - C(aws), C(azurerm), C(google).
Copy link
Member

Choose a reason for hiding this comment

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

#146 would add the ability to use any provider.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, nice, but again, until such time, the text I've suggested above would have saved me a few hours to realise the module does nothing for me when using a provider not in that currently supported group.

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

2 participants