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

Place default ID attribute in Read Only group #46

Closed
wants to merge 7 commits into from

Conversation

nmuesch
Copy link
Contributor

@nmuesch nmuesch commented Mar 4, 2021

Fixes #45

If the user hasn't specified the "ID" attribute in their schema, move it to the "Read Only" group so its not assumed to be a field that can be set explicitly in the terraform config.

@hashicorp-cla
Copy link

hashicorp-cla commented Mar 4, 2021

CLA assistant check
All committers have signed the CLA.

@detro detro added this to the v0.8.0 milestone Apr 26, 2022
Copy link
Contributor

@detro detro left a comment

This is a nice improvement to an annoying problem.

Will likely need to land in 0.8.0. Can you please add a BUG FIX entry in the CHANGELOG?

@@ -161,7 +158,13 @@ func writeBlockChildren(w io.Writer, parents []string, block *tfjson.SchemaBlock
childBlock := block.NestedBlocks[n]
childAtt := block.Attributes[n]
for i, gf := range groupFilters {
if gf.filter(childBlock, childAtt) {
if n == "id" && childAtt.Description == "" {
Copy link
Contributor

@detro detro Apr 26, 2022

Choose a reason for hiding this comment

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

Nit: I'd do a lowercase comparison with id, just to stay on the safe side

@@ -161,7 +158,13 @@ func writeBlockChildren(w io.Writer, parents []string, block *tfjson.SchemaBlock
childBlock := block.NestedBlocks[n]
childAtt := block.Attributes[n]
for i, gf := range groupFilters {
if gf.filter(childBlock, childAtt) {
if n == "id" && childAtt.Description == "" {
if gf.name == "Read Only" {
Copy link
Contributor

@detro detro Apr 26, 2022

Choose a reason for hiding this comment

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

Instead of creating a new field name for group filter, just to support this comparison, wouldn't it be easier to just do a string contains Read-Only?

Copy link
Contributor Author

@nmuesch nmuesch Apr 26, 2022

Choose a reason for hiding this comment

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

I changed the current implementation to check this. I think my original rationale was trying to give it an enum style ID that could be referenced (in case it needed to be for any other purposes) But I'm totally fine just checking against an existing field.

@detro detro self-assigned this Apr 26, 2022
@nmuesch nmuesch requested a review from a team as a code owner Apr 26, 2022
@nmuesch
Copy link
Contributor Author

nmuesch commented Apr 26, 2022

Hey @detro Thanks for the review, I brought this up to date with the main branch and believe I've addressed your comments.

I haven't been working on terraform in a while though, so while I tried to do a quick sanity check that this still works as expected, it'd be great if someone were able to give this a test on their side.

Copy link
Contributor

@detro detro left a comment

I think I might have identified an issue, and also provided some suggestions.

Also, please ensure you have tried these changes, as the testing facilities in this repo are a bit limited, so manual verification is key.

@@ -225,6 +225,13 @@ nameLoop:
}
} else if childAtt, ok := block.Attributes[n]; ok {
for i, gf := range groupFilters {
if strings.ToLower(n) == "id" && childAtt.Description == "" {
if gf.topLevelTitle == "Read-Only" {
Copy link
Contributor

@detro detro Apr 28, 2022

Choose a reason for hiding this comment

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

Did you verify this? Because I think this won't match anything: I believe the string is ### Read-Only.

It would be even better if the elements of groupFilters above existed as variables themselves, and so you could directly match that and avoid bugs out of string mismatches (now and in the future).

Copy link
Contributor

@OJFord OJFord Apr 30, 2022

Choose a reason for hiding this comment

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

That's correct, I have it working with that change (see below).

@@ -225,6 +225,13 @@ nameLoop:
}
} else if childAtt, ok := block.Attributes[n]; ok {
for i, gf := range groupFilters {
if strings.ToLower(n) == "id" && childAtt.Description == "" {
if gf.topLevelTitle == "Read-Only" {
childAtt.Description = "The ID of this resource."
Copy link
Contributor

@detro detro Apr 28, 2022

Choose a reason for hiding this comment

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

I noticed you are not removing anymore the if here that sets the description for id. What changed?

Copy link
Contributor

@OJFord OJFord Apr 30, 2022

Choose a reason for hiding this comment

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

Yes it's also necessary to remove that, and I'm not sure it's worth checking for empty description here anyway?

Copy link
Contributor

@detro detro May 3, 2022

Choose a reason for hiding this comment

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

If we don't do that, all descriptions currently set on IDs will be overridden by this change, making it impossible for users to tailor-make the description they want.

Copy link
Contributor

@OJFord OJFord May 3, 2022

Choose a reason for hiding this comment

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

But then only the description setting should be inside this condition, not the appending to Read-Only group too?

CHANGELOG.md Outdated Show resolved Hide resolved
@OJFord
Copy link
Contributor

OJFord commented Apr 30, 2022

Also, please ensure you have tried these changes, as the testing facilities in this repo are a bit limited, so manual verification is key.

I haven't been working on terraform in a while though, so while I tried to do a quick sanity check that this still works as expected, it'd be great if someone were able to give this a test on their side.

Hi, many thanks for working on this @nmuesch! I found your original report after a user report in OJFord/terraform-provider-wireguard#8.

I've just tested your branch against that repo:

$ (cd terraform-plugin-docs && go build cmd/tfplugindocs)
$ (cd terraform-provider-wireguard && rm -r docs && ../terraform-plugin-docs/tfplugindocs generate && git status)

and unfortunately the docs are generated exactly as they were by v0.7.0 - i.e. with an optional ID.

However, with a couple of @detro's points above, and also a missing but crucial else, it works great:

diff --git a/docs/data-sources/config_document.md b/docs/data-sources/config_document.md
index 094d39d..f5dc77e 100644
--- a/docs/data-sources/config_document.md
+++ b/docs/data-sources/config_document.md
@@ -59,7 +59,6 @@ output "peer1" {
 - `addresses` (Set of String) IPs (or CIDR) to be assigned to the interface. (`wg-quick`/apps only.)
 - `dns` (Set of String) IPs or hostnames of Domain Name Servers to set as the interface's DNS search domains. (`wg-quick`/apps only.)
 - `firewall_mark` (String) A 32-bit fwmark for outgoing packets.
-- `id` (String) The ID of this resource.
 - `listen_port` (Number) .
 - `mtu` (Number) Manual MTU to override automatic discovery. (`wg-quick`/apps only.)
 - `peer` (Block List) (see [below for nested schema](#nestedblock--peer))
@@ -72,6 +71,7 @@ output "peer1" {
 ### Read-Only
 
 - `conf` (String, Sensitive) The rendered config document.
+- `id` (String) The ID of this resource.
 
 <a id="nestedblock--peer"></a>
 ### Nested Schema for `peer`
diff --git a/docs/resources/asymmetric_key.md b/docs/resources/asymmetric_key.md
index 12eb870..cc49549 100644
--- a/docs/resources/asymmetric_key.md
+++ b/docs/resources/asymmetric_key.md
@@ -34,11 +34,11 @@ output "wg_private_key" {
 ### Optional
 
 - `bind` (String) A string to tie the lifecycle to, e.g. the terraform ID of another resource.
-- `id` (String) The ID of this resource.
 - `private_key` (String, Sensitive) A supplied WireGuard private key. By default one is generated.
 
 ### Read-Only
 
+- `id` (String) The ID of this resource.
 - `public_key` (String) The public key corresponding to the (given or generated) private key.
 
 
diff --git a/docs/resources/preshared_key.md b/docs/resources/preshared_key.md
index 8f73028..104fe50 100644
--- a/docs/resources/preshared_key.md
+++ b/docs/resources/preshared_key.md
@@ -26,12 +26,9 @@ output "wg_preshared_key" {
 <!-- schema generated by tfplugindocs -->
 ## Schema
 
-### Optional
-
-- `id` (String) The ID of this resource.
-
 ### Read-Only
 
+- `id` (String) The ID of this resource.
 - `key` (String, Sensitive) Additional layer of symmetric-key cryptography to be mixed into the already existing public-key cryptography, for post-quantum resistance.
 
 

Here's my diff to get it working, based on your branch at the time of writing, 5e66e53:

diff --git a/schemamd/render.go b/schemamd/render.go
index c5e7db8..03af460 100644
--- a/schemamd/render.go
+++ b/schemamd/render.go
@@ -70,10 +70,6 @@ func writeAttribute(w io.Writer, path []string, att *tfjson.SchemaAttribute, gro
 		return nil, err
 	}
 
-	if name == "id" && att.Description == "" {
-		att.Description = "The ID of this resource."
-	}
-
 	if att.AttributeNestedType == nil {
 		err = WriteAttributeDescription(w, att, false)
 	} else {
@@ -225,14 +221,13 @@ nameLoop:
 			}
 		} else if childAtt, ok := block.Attributes[n]; ok {
 			for i, gf := range groupFilters {
-				if strings.ToLower(n) == "id" && childAtt.Description == "" {
-					if gf.topLevelTitle == "Read-Only" {
+				if strings.ToLower(n) == "id" {
+					if gf.topLevelTitle == "### Read-Only" {
 						childAtt.Description = "The ID of this resource."
 						groups[i] = append(groups[i], n)
 						continue nameLoop
 					}
-				}
-				if gf.filterAttribute(childAtt) {
+				} else if gf.filterAttribute(childAtt) {
 					groups[i] = append(groups[i], n)
 					continue nameLoop
 				}

Thanks again for this nmuesch. 🙂

schemamd/render.go Outdated Show resolved Hide resolved
detro and others added 3 commits May 3, 2022
Co-authored-by: Oliver Ford <dev.github@ojford.com>
@nmuesch
Copy link
Contributor Author

nmuesch commented May 3, 2022

Hey, apologies for my slow response. I did attempt to test things, but I've been a bit out of golang for a little while now and my test environment may have not been using the newly built version I was expecting.

Anyway, thanks to both of you for continuing work on this and getting it past the finish line 🙇

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.

ID attribute marked as Optional in generated documentation
4 participants