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 updates #102

Merged
merged 6 commits into from
Apr 16, 2021
Merged

Doc updates #102

merged 6 commits into from
Apr 16, 2021

Conversation

bcmdarroch
Copy link
Contributor

🛠️ Description

This PR updates the warning messages on resources with sensitive input/output. It also adds a warning about the bug released in v0.4.0 and patched in v0.4.1.

@bcmdarroch bcmdarroch self-assigned this Apr 14, 2021
@bcmdarroch bcmdarroch requested a review from a team April 14, 2021 21:55
Copy link
Contributor

@roaks3 roaks3 left a comment

Choose a reason for hiding this comment

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

Mostly looks good, but had a few comments, and I think the Consul root token docs need a few fixes

@@ -20,6 +20,8 @@ data "hcp_aws_transit_gateway_attachment" "test" {
}
```

~> **Security Notice** This resource contains sensitive input. Please see this [list of recommendations](https://www.terraform.io/docs/language/state/sensitive-data.html) for storing sensitive information in Terraform.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe the data source actually contains any sensitive inputs, so this message might be confusing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops you're right, thought the ARN might be an output

@@ -9,12 +9,18 @@ description: |-

The HCP provider provides resources to manage [HashiCorp Cloud Platform](https://cloud.hashicorp.com/) (HCP) resources.

~> **Known Issue** There is an issue with v0.4.0 of the HCP Provider in which existing Consul clusters that do not specify size will be force-recreated on the next Apply. Please upgrade to the patch v0.4.1 to avoid this issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I understand the intent of this message, but from the user's perspective it may be a little weird. What we will end up with is a v0.4.2 when this gets released, which will mention that the user should use v0.4.1, because v0.4.0 has an issue. But then when the user selects v0.4.0 or v0.4.1 from the version selector, they will not see any messages related to known issues. I'm wondering if we will instead need to rely on a more generic message if we really need something like this, eg. Please refer our [CHANGELOG]() for critical fixes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the idea of adding the generic message, I can do that. @xargs-P wants to keep the specific 0.4.0 warning in for 30 days, then we'll remove it. We're adding the same 30-day warning to the CLI (which will show up when the provider initializes). We want to be extra noisy about that issue, since it could result in data loss.

docs/index.md Outdated
## Authenticating with HCP

The HCP provider supports authentication via a Client ID and a Client Secret. The [authentication guide](guides/auth.md) describes how to obtain client credentials.

## Example Usage

Most HCP resources have a dependency on the Hashicorp Virtual Network (HVN) resource.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: IMO this is so fundamental to Terraform usage that it would be extraneous information for the average user (I'm not aware of any other providers that include a note like this), and for Terraform beginners, we probably need to be directing them to more comprehensive guides to be brought up to speed


{{ .Description | trimspace }}

The cluster root token resource is the token used to bootstrap the cluster's ACL system. Using this resource to create a new root token for a cluster will invalidate the consul root token accessor id and Consul root token secret id properties of the cluster.
Copy link
Contributor

Choose a reason for hiding this comment

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

This line seems like a duplicate of the Description above?


## Example Usage

{{ tffile "examples/resources/hcp_vault_cluster_admin_token/resource.tf" }}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this one needs to be hcp_consul_cluster_root_token instead of hcp_vault_cluster_admin_token

@bcmdarroch bcmdarroch merged commit 458a211 into main Apr 16, 2021
@bcmdarroch bcmdarroch deleted the doc-updates branch April 16, 2021 21:23
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.

2 participants