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

Allowing single word resources to use templates #147

Merged

Conversation

bendbennett
Copy link
Contributor

@bendbennett bendbennett commented Jun 6, 2022

Currently, it is not possible to have single word resources or data sources (e.g., http) use templates in the generation of docs. This PR addresses that by allowing single word resources and data sources to use a matching template (e.g., http data source using http.md.tmpl template).

An error is now returned if a resource cannot be found using either the provider short name or the provider short name concatenated with the template file name.

@bendbennett bendbennett requested a review from a team as a code owner June 6, 2022 15:28
bflad
bflad previously approved these changes Jun 6, 2022
Copy link
Member

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Sad that we need this, but looks good to me 🚀

// are identical after file extensions have been stripped from the
// latter. This allows single word resources (e.g., http) to use
// templates (e.g., http.md.tmpl).
func resourceName(shortName, relFile string) string {
Copy link
Member

Choose a reason for hiding this comment

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

Could we add a quick unit test for this?

@@ -414,7 +414,7 @@ func (g *generator) renderStaticWebsite(providerName string, providerSchema *tfj
g.infof("rendering %q", rel)
switch relDir {
case "data-sources/":
resName := shortName + "_" + removeAllExt(relFile)
resName := resourceName(shortName, relFile)
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: isn't shortName the Provider shortname here?

In the case of the HTTP provider (through which you identified the issue), I believe the http refers to the resource. Otherwise if, for example, you decide to create a second resource in this provider (say, for the sake of it, https), you won't be able to match it OR it would match the wrong file.

I'll leave a comment in the unit test below.

templateFileName string
expectedResourceName string
}{
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add another case where you have a resource named (just for kicks) https, using the same format used by the http provider?

I say this because the http provider is here not respecting the convention: <provider>_<resource> when exposed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@detro you mean along the lines of:

{
	"provider short name different from template file name",
	"https",
	"http.md.tmpl",
	"https_http",
},

Copy link
Contributor

Choose a reason for hiding this comment

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

{
	"provider short name different from template file name",
	"http",
	"https.md.tmpl",
	"https",
},

What I'm expecting is that this test would fail, because the provider name is http, but the resource name is https.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@detro I've refactored following discussion.

@bendbennett
Copy link
Contributor Author

bendbennett commented Jun 6, 2022

Sad that we need this, but looks good to me 🚀

If the http provider is the only provider that doesn't respect <provider>_<resource> we could make a breaking change to http and change it to something like http_request?

@detro
Copy link
Contributor

detro commented Jun 6, 2022

I'd be a BIG fan of http_request.

@bendbennett bendbennett requested review from detro and bflad June 9, 2022 15:53
bflad
bflad previously approved these changes Jun 9, 2022
Copy link
Member

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Looks good to me 🚀

Comment on lines 59 to 68
{
"provider short name concatenated with same template file name matches schema name",
map[string]*tfjson.Schema{
"tls_tls": {},
},
"tls",
"tls.md.tmpl",
&tfjson.Schema{},
"tls_tls",
},
Copy link
Member

Choose a reason for hiding this comment

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

Nit: While Go allows you to omit field names, I personally find it a little more difficult to read the structs once the struct definition is "beyond the fold" since its based on ordering. e.g. my brain is like what is the 3rd field defined as "tls" here? I have to scroll up, see providerShortName, scroll back down

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point. I forget that GoLand is doing this for me:

image

Have added field names and refactored to use map[string]struct{} for the test cases.

# 0.9.0 (June 1, 2022)

NEW FEATURES:

* cmd/tflugindocs: Additional CLI arguments `provider-name`, `rendered-provider-name`, `rendered-website-dir`, `examples-dir`, `website-temp-dir`, and `website-source-dir`. These allow to further customise generated doc ([#95](https://github.com/hashicorp/terraform-plugin-docs/pull/95)).
* cmd/tfplugindocs: Additional CLI arguments `provider-name`, `rendered-provider-name`, `rendered-website-dir`, `examples-dir`, `website-temp-dir`, and `website-source-dir`. These allow to further customise generated doc ([#95](https://github.com/hashicorp/terraform-plugin-docs/pull/95)).
Copy link
Contributor

Choose a reason for hiding this comment

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

those are definitely typos I made 🤦

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries. I like that we're keeping an eye out for each other 👍

Copy link
Contributor

@detro detro left a comment

Choose a reason for hiding this comment

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

👩‍🍳 💋

@bendbennett bendbennett merged commit cbd432b into main Jun 10, 2022
@bendbennett bendbennett deleted the bendbennett/allow-single-word-resources-data-sources branch June 10, 2022 12:27
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.

None yet

3 participants