-
Notifications
You must be signed in to change notification settings - Fork 178
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
chore: Improves schema Description / MarkdownDescription #2605
Conversation
88f9105
to
e7cf7c4
Compare
APIx bot: a message has been sent to Docs Slack channel |
if ptr.Kind() != reflect.Ptr { | ||
panic("not ptr, please fix caller") | ||
} | ||
v := ptr.Elem() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they're short var names but it's in line with conventional naming when doing reflection
} | ||
v := ptr.Elem() | ||
if v.Kind() != reflect.Struct { | ||
panic("not struct, please fix caller") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added multiple panic to make sure it works as expected.
These panics will be caught in unit tests so they won't make it to master, e.g. in TestResourceSchemas
newPtr := reflect.New(v.Type()) | ||
newPtr.Elem().Set(v) | ||
updateAttr(newPtr.Interface()) | ||
f.SetMapIndex(k, newPtr.Elem()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maps are not addressable so a new element is created and udpated
@@ -0,0 +1,73 @@ | |||
package conversion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
curious if we could unit test this, but might not be worth it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could, although TestResourceSchemas is testing it indirectly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@EspenAlbert at the end I created some unit tests here 318c46c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
Almost net negative PR 😅 +165 −143
GetMarkdownDescription() string | ||
} | ||
|
||
func checkDescriptor(name string, d descriptor, diagnostics *diag.Diagnostics) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost net negative PR 😅 +165 −143
it'll be negative in the future when more schemas are added ;-)
no doc review needed as it's only deleting duplicated doc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* master: (27 commits) chore: Bump github.com/hashicorp/terraform-plugin-framework (#2632) chore: Bump tj-actions/verify-changed-files (#2629) chore: Bump peter-evans/create-pull-request from 7.0.3 to 7.0.5 (#2628) chore: Bump go.mongodb.org/atlas-sdk/v20240805004 (#2630) chore: Bump github.com/hashicorp/terraform-plugin-go (#2631) fix: Makes `mongodbatlas_alert_configuration` Datadog acceptance tests non-parallel (#2626) chore: Improves schema Description / MarkdownDescription (#2605) fix doc (#2619) chore: Updates CHANGELOG.md header for v1.20.0 release chore: Updates examples link in index.md for v1.20.0 release chore: Updates CHANGELOG.md for #2617 chore: Removes 1.20 deprecations (#2617) chore: Updates CHANGELOG.md for #2603 fix: Updates `integration_id` in `mongodbatlas_alert_configuration` resource to be Optional+Computed (#2603) chore: fixed examples broken links (#2613) adjust instance size of sharded cluster to M30 and above (#2615) doc: remove EOL sentences (#2616) Dialed in the version (#2614) chore: Updates CHANGELOG.md for #2604 fix: Supports using decimal in cluster+adv_cluster advanced_configuration `oplog_min_retention_hours` (#2604) ... # Conflicts: # go.mod # go.sum
Description
Improves schema Description / MarkdownDescription.
Implementation details:
Link to any related issue(s): CLOUDP-274028
Type of change:
Required Checklist:
Further comments