-
Notifications
You must be signed in to change notification settings - Fork 68
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
fix: #177 use rendered provider name in default template #178
base: main
Are you sure you want to change the base?
Conversation
@@ -245,13 +245,15 @@ Import is supported using the following syntax: | |||
|
|||
const defaultProviderTemplate providerTemplate = `--- | |||
` + frontmatterComment + ` | |||
page_title: "{{.ProviderShortName}} Provider" | |||
{{- $name := .ProviderShortName}} | |||
{{- if ne .ProviderName .RenderedProviderName }}{{ $name = .RenderedProviderName}}{{ end }} |
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.
i believe the logic is incorrect here as this will always attempt to use .RenderedProviderName
since .ProviderName
is always going to differ from .RenderedProviderName
.
i think what want is more like this:
{- $name := .ProviderShortName}}
{{- if .RenderedProviderName }}{{ $name = .RenderedProviderName }}{{ end }}
page_title: "{{ $name }} Provider"
(this is also more explicit by checking if the rendered provider name value is set, not just different)
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.
I think .RenderedProviderName
is always set, and its default value is .ProviderName
. Here's the reference:
terraform-plugin-docs/internal/provider/generate.go
Lines 123 to 125 in 2f535d3
if g.renderedProviderName == "" { | |
g.renderedProviderName = providerName | |
} |
Feel free to try it out!
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.
@jacobbednarz have you had the chance to check again?
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.
i haven't, however, being explicit in the template makes this clear what the expected outcome is instead of relying on overrides within the generation process.
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.
I totally agree! Actually, the solution you proposed was the first one that came to my mind as well. Unfortunately, it just doesn't work because the .RenderedProviderName
is never unset, as shown in the previous comment.
Thus, this PR will keep the default behaviour the same as it was before, and it overrides the provider name in the template only if the specific variable has been set.
5ef5342
to
ed269f3
Compare
ed269f3
to
d78a366
Compare
d78a366
to
925bfc4
Compare
925bfc4
to
d146b3c
Compare
This is to solve #177 as suggested here: #177 (comment)
If --rendered-provider-name is set, the default template will use it. If not, it will continue to use previous value