Skip to content

Commit

Permalink
Fix docs generator parent module computation (#15035)
Browse files Browse the repository at this point in the history
<!--- 
Thanks so much for your contribution! If this is your first time
contributing, please ensure that you have read the
[CONTRIBUTING](https://github.com/pulumi/pulumi/blob/master/CONTRIBUTING.md)
documentation.
-->

# Description

Fixes #14820
Fixes #14821

Fixes a bug in docs generator parent module computation that used to
lead to duplicate files panics and undesirable documentation URL layout
for affected providers such as
[pulumi/fortios](https://github.com/pulumiverse/pulumi-fortios/issues).

Joint work with @tmeckel .

## Checklist

- [ ] I have run `make tidy` to update any new dependencies
- [ ] I have run `make lint` to verify my code passes the lint check
  - [ ] I have formatted my code using `gofumpt`

<!--- Please provide details if the checkbox below is to be left
unchecked. -->
- [ ] I have added tests that prove my fix is effective or that my
feature works
<!--- 
User-facing changes require a CHANGELOG entry.
-->
- [ ] I have run `make changelog` and committed the
`changelog/pending/<file>` documenting my change
<!--
If the change(s) in this PR is a modification of an existing call to the
Pulumi Cloud,
then the service should honor older versions of the CLI where this
change would not exist.
You must then bump the API version in
/pkg/backend/httpstate/client/api.go, as well as add
it to the service.
-->
- [ ] Yes, there are changes in this PR that warrants bumping the Pulumi
Cloud API version
<!-- @pulumi employees: If yes, you must submit corresponding changes in
the service repo. -->

---------

Co-authored-by: Thomas Meckel <tmeckel@users.noreply.github.com>
  • Loading branch information
t0yv0 and tmeckel committed Feb 20, 2024
1 parent 0f8d116 commit 5f8ca93
Show file tree
Hide file tree
Showing 3 changed files with 269 additions and 14 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
changes:
- type: fix
scope: docs
description: Fixes docs generator parent module computation
51 changes: 37 additions & 14 deletions pkg/codegen/docs/gen.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ import (
"github.com/pulumi/pulumi/pkg/v3/codegen/python"
"github.com/pulumi/pulumi/pkg/v3/codegen/schema"
"github.com/pulumi/pulumi/sdk/v3/go/common/slice"
"github.com/pulumi/pulumi/sdk/v3/go/common/tokens"
"github.com/pulumi/pulumi/sdk/v3/go/common/util/contract"
)

Expand Down Expand Up @@ -2016,33 +2017,38 @@ func (dctx *docGenContext) getMod(
add bool,
) *modContext {
modName := pkg.TokenToModule(token)
mod, ok := modules[modName]
return dctx.getModByModName(pkg, tokens.ModuleName(modName), tokenPkg, modules, tool, add)
}

func (dctx *docGenContext) getModByModName(
pkg schema.PackageReference,
modName tokens.ModuleName,
tokenPkg schema.PackageReference,
modules map[string]*modContext,
tool string,
add bool,
) *modContext {
mod, ok := modules[string(modName)]
if !ok {
mod = &modContext{
pkg: pkg,
mod: modName,
mod: string(modName),
tool: tool,
docGenContext: dctx,
}

if modName != "" && codegen.PkgEquals(tokenPkg, pkg) {
parentName := path.Dir(modName)
// If the parent name is blank, it means this is the package-level.
if parentName == "." || parentName == "" {
parentName = ":index:"
} else {
parentName = ":" + parentName + ":"
}
parent := dctx.getMod(pkg, parentName, tokenPkg, modules, tool, add)
if add {
parentName, hasParent := parentModule(modName)
if add && hasParent {
parent := dctx.getModByModName(pkg, parentName, tokenPkg, modules, tool, add)
parent.children = append(parent.children, mod)
}
}

// Save the module only if we're adding and it's for the current package.
// This way, modules for external packages are not saved.
if add && tokenPkg == pkg {
modules[modName] = mod
modules[string(modName)] = mod
}
}
return mod
Expand Down Expand Up @@ -2206,6 +2212,11 @@ func (dctx *docGenContext) generatePackage(tool string, pkg *schema.Package) (ma
return files, nil
}

const (
// "" indicates the top-most module.
topMostModule tokens.ModuleName = ""
)

// GeneratePackageTree returns a navigable structure starting from the top-most module.
func (dctx *docGenContext) generatePackageTree() ([]PackageTreeItem, error) {
if dctx.modules() == nil {
Expand All @@ -2215,8 +2226,7 @@ func (dctx *docGenContext) generatePackageTree() ([]PackageTreeItem, error) {
defer glog.Flush()

var packageTree []PackageTreeItem
// "" indicates the top-most module.
if rootMod, ok := dctx.modules()[""]; ok {
if rootMod, ok := dctx.modules()[string(topMostModule)]; ok {
tree, err := generatePackageTree(*rootMod)
if err != nil {
glog.Errorf("Error generating the package tree for package: %v", err)
Expand Down Expand Up @@ -2259,3 +2269,16 @@ func GeneratePackage(tool string, pkg *schema.Package) (map[string][]byte, error
func GeneratePackageTree() ([]PackageTreeItem, error) {
return defaultContext.generatePackageTree()
}

// Returns the parent module, if available. For top-level modules the parent is a special
// topMostModule. For topMostModule itself there is no parent and this function returns false.
func parentModule(modName tokens.ModuleName) (tokens.ModuleName, bool) {
switch {
case modName == topMostModule:
return "", false
case strings.Contains(string(modName), tokens.QNameDelimiter):
return tokens.ModuleName(tokens.QName(modName).Namespace()), true
default:
return topMostModule, true
}
}
228 changes: 228 additions & 0 deletions pkg/codegen/docs/package_tree_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,11 @@
package docs

import (
"encoding/json"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/pulumi/pulumi/pkg/v3/codegen/schema"
)
Expand Down Expand Up @@ -68,3 +70,229 @@ func TestGeneratePackageTree(t *testing.T) {
assert.Equal(t, entryTypeFunction, children[1].Type)
})
}

// Original issues: pulumi/pulumi#14821, pulumi/pulumi#14820.
func TestGeneratePackageTreeNested(t *testing.T) {
t.Parallel()

type testCase struct {
name string
spec schema.PackageSpec
expect string
}

testCases := []testCase{
{
"index top module",
schema.PackageSpec{
Name: "testindex",
Version: "0.0.1",
Resources: map[string]schema.ResourceSpec{
"testindex:index:Resource": {},
},
},
`[
{
"name": "Provider",
"type": "resource",
"link": "provider"
},
{
"name": "Resource",
"type": "resource",
"link": "resource"
}
]`,
},
{
"random",
schema.PackageSpec{
Name: "random",
Version: "0.0.1",
Resources: map[string]schema.ResourceSpec{
"random:index/randomId:RandomId": {},
"random:index/randomPassword:RandomPassword": {},
},
},
`
[
{
"name": "Provider",
"type": "resource",
"link": "provider"
},
{
"name": "RandomId",
"type": "resource",
"link": "randomid"
},
{
"name": "RandomPassword",
"type": "resource",
"link": "randompassword"
}
]`,
},
{
"14820",
schema.PackageSpec{
Name: "fortios",
Version: "0.0.1",
Resources: map[string]schema.ResourceSpec{
"fortios:router/bgp:Bgp": {},
"fortios:router/bgp/network:Network": {},
},
},
`
[
{
"name": "router",
"type": "module",
"link": "router/",
"children": [
{
"name": "bgp",
"type": "module",
"link": "bgp/",
"children": [
{
"name": "Network",
"type": "resource",
"link": "network"
}
]
},
{
"name": "Bgp",
"type": "resource",
"link": "bgp"
}
]
},
{
"name": "Provider",
"type": "resource",
"link": "provider"
}
]`,
},
{
"14821",
schema.PackageSpec{
Name: "fortios",
Version: "0.0.1",
Resources: map[string]schema.ResourceSpec{
"fortios:log/syslogd/v2/filter:Filter": {},
"fortios:log/syslogd/v2/overridefilter:Overridefilter": {},
"fortios:log/syslogd/v2/overridesetting:Overridesetting": {},
"fortios:log/syslogd/v2/setting:Setting": {},
},
},
`
[
{
"name": "log",
"type": "module",
"link": "log/",
"children": [
{
"name": "syslogd",
"type": "module",
"link": "syslogd/",
"children": [
{
"name": "v2",
"type": "module",
"link": "v2/",
"children": [
{
"name": "Filter",
"type": "resource",
"link": "filter"
},
{
"name": "Overridefilter",
"type": "resource",
"link": "overridefilter"
},
{
"name": "Overridesetting",
"type": "resource",
"link": "overridesetting"
},
{
"name": "Setting",
"type": "resource",
"link": "setting"
}
]
}
]
}
]
},
{
"name": "Provider",
"type": "resource",
"link": "provider"
}
]`,
},
}

prep := func(t *testing.T, tc testCase) (*docGenContext, *schema.Package) {
t.Helper()

schemaPkg, err := schema.ImportSpec(tc.spec, nil)
assert.NoError(t, err, "importing spec")

c := newDocGenContext()
c.initialize("test", schemaPkg)

return c, schemaPkg
}

for _, tc := range testCases {
tc := tc

for _, mv := range []string{"", "(.*)(?:/[^/]*)"} {
name := tc.name
if mv != "" {
tc.spec.Meta = &schema.MetadataSpec{
ModuleFormat: mv,
}
name += "-withModuleFormat"
} else {
name += "-withoutModuleFormat"
}
t.Run(name, func(t *testing.T) {
t.Parallel()

c, _ := prep(t, tc)

items, err := c.generatePackageTree()
require.NoError(t, err)

data, err := json.MarshalIndent(items, "", " ")
require.NoError(t, err)

t.Logf("%s", string(data))

require.JSONEq(t, tc.expect, string(data))
})

t.Run(name+"/generatePackage", func(t *testing.T) {
t.Parallel()

c, schemaPkg := prep(t, tc)

files, err := c.generatePackage("test", schemaPkg)
require.NoError(t, err)

for f := range files {
t.Logf("+ %v", f)
}
})
}
}
}

0 comments on commit 5f8ca93

Please sign in to comment.