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.

Sign up for GitHub

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

Jump to bottom

Place default ID attribute in Read Only group #46

Closed
wants to merge 7 commits into from
Closed

Place default ID attribute in Read Only group #46

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e6cc7a3
Place default ID attribute in Read Only group
nmuesch Mar 4, 2021
40b0963
Remove extra check of empty ID description
nmuesch Mar 4, 2021
96fb961
Merge branch 'main' into nick/default_id
nmuesch Apr 26, 2022
5e66e53
Address feedback
nmuesch Apr 26, 2022
af5eaa6
Update CHANGELOG.md
detro May 3, 2022
3ad1834
Update schemamd/render.go
detro May 3, 2022
9ee2af1
Update render.go
May 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 5 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Expand Up @@ -3,7 +3,11 @@
ENHANCEMENTS:

* template functions: Added `split` to help separating a string into substrings ([#70](https://github.com/hashicorp/terraform-plugin-docs/pull/70)).


BUG FIXES:

* schemamd: Place the default "ID" attribute as a "Read Only" attribute, unless there's a defined description ([#46](https://github.com/hashicorp/terraform-plugin-docs/pull/46)).
detro marked this conversation as resolved.
Show resolved Hide resolved

# 0.7.0 (March 15, 2022)

ENHANCEMENTS:
Expand Down
7 changes: 7 additions & 0 deletions schemamd/render.go
View file
Open in desktop
Expand Up @@ -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

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

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).

childAtt.Description = "The ID of this resource."
Copy link
Contributor

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

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

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

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?

groups[i] = append(groups[i], n)
continue nameLoop
}
}
if gf.filterAttribute(childAtt) {
detro marked this conversation as resolved.
Show resolved Hide resolved
groups[i] = append(groups[i], n)
continue nameLoop
Expand Down