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

Allowing single word resources to use templates #147

Merged
merged 5 commits into from Jun 10, 2022
Merged

Allowing single word resources to use templates #147

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b4b63ec
Allowing single word resources to use templates
bendbennett Jun 6, 2022
9377652
Updating CHANGELOG.md
bendbennett Jun 6, 2022
27bf553
Add unit test for resourceName
bendbennett Jun 6, 2022
b0001a4
Returning error if resource not found
bendbennett Jun 9, 2022
d6cc333
Adding field names to test structs
bendbennett Jun 10, 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
10 changes: 8 additions & 2 deletions CHANGELOG.md
View file
Open in desktop
@@ -1,12 +1,18 @@
# 0.9.1 (Unreleased)

BUG FIXES:

* cmd/tfplugindocs: Allow single word resources to use templates ([147](https://github.com/hashicorp/terraform-plugin-docs/pull/147)).

# 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 👍


ENHANCEMENTS:

* cmd/tflugindocs: Implemented usage output (i.e. `--help`) for `generate` and `validate` commands ([#95](https://github.com/hashicorp/terraform-plugin-docs/pull/95)).
* cmd/tfplugindocs: Implemented usage output (i.e. `--help`) for `generate` and `validate` commands ([#95](https://github.com/hashicorp/terraform-plugin-docs/pull/95)).

# 0.8.1 (May 10, 2022)

Expand Down
4 changes: 2 additions & 2 deletions internal/provider/generate.go
View file
Open in desktop
Expand Up @@ -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.

resSchema, ok := providerSchema.DataSourceSchemas[resName]
if ok {
tmpl := resourceTemplate(tmplData)
Expand All @@ -429,7 +429,7 @@ func (g *generator) renderStaticWebsite(providerName string, providerSchema *tfj
return nil
}
case "resources/":
resName := shortName + "_" + removeAllExt(relFile)
resName := resourceName(shortName, relFile)
resSchema, ok := providerSchema.ResourceSchemas[resName]
if ok {
tmpl := resourceTemplate(tmplData)
Expand Down
1 change: 1 addition & 0 deletions internal/provider/template.go
View file
Open in desktop
Expand Up @@ -8,6 +8,7 @@ import (
"text/template"

tfjson "github.com/hashicorp/terraform-json"

"github.com/hashicorp/terraform-plugin-docs/internal/mdplain"
"github.com/hashicorp/terraform-plugin-docs/internal/tmplfuncs"
"github.com/hashicorp/terraform-plugin-docs/schemamd"
Expand Down
12 changes: 12 additions & 0 deletions internal/provider/util.go
View file
Open in desktop
Expand Up @@ -52,6 +52,18 @@ func removeAllExt(file string) string {
}
}

// resourceName determines whether the shortname and the relFile
// 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?

if shortName == removeAllExt(relFile) {
return shortName
}

return shortName + "_" + removeAllExt(relFile)
}

func writeFile(path string, data string) error {
dir, _ := filepath.Split(path)
err := os.MkdirAll(dir, 0755)
Expand Down
38 changes: 38 additions & 0 deletions internal/provider/util_test.go
View file
Open in desktop
@@ -0,0 +1,38 @@
package provider

import (
"testing"

"github.com/google/go-cmp/cmp"
)

func Test_resourceName(t *testing.T) {
cases := []struct {
name string
providerShortName string
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.

"provider short name same as template file name",
"http",
"http.md.tmpl",
"http",
},
{
"provider short name different to template file name",
"tls",
"cert_request.md.tmpl",
"tls_cert_request",
},
}

for _, c := range cases {
t.Run(c.name, func(t *testing.T) {
actualResourceName := resourceName(c.providerShortName, c.templateFileName)
if !cmp.Equal(c.expectedResourceName, actualResourceName) {
t.Errorf("expected: %s, got: %s", c.expectedResourceName, actualResourceName)
}
})
}
}