Skip to content

Commit

Permalink
Add a workflow to enforce new docs redirects
Browse files Browse the repository at this point in the history
Add a `docpaths` workflow to the bot to ensure that any renamed or
deleted docs pages accompany a new redirect in the docs config file
(`config.json`). This way, we can prevent docs changes from introducing
404s, whether because of delays in search engine indexing or broken
links from Teleport-owned sites.

The workflow takes a path to a `config.json` file, loads the redirect
configuration, and checks whether all renamed and deleted pages
correspond to a redirect. This change adds a `docpaths` value of the
`workflow` flag and an optional `teleport-path` flag for the path to
a `gravitational/teleport` clone so the workflow can locate a docs
configuration file.
  • Loading branch information
ptgott committed Nov 19, 2024
1 parent 4bd2c6c commit de56c4f
Show file tree
Hide file tree
Showing 4 changed files with 301 additions and 26 deletions.
111 changes: 111 additions & 0 deletions bot/internal/bot/docpaths.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
package bot

import (
"context"
"encoding/json"
"errors"
"fmt"
"os"
"path"
"strings"

"github.com/gravitational/shared-workflows/bot/internal/github"
"github.com/gravitational/trace"
)

// DocsConfig represents the structure of the config.json file found in the docs
// directory of a Teleport repo. We only use Redirects, so other fields have the
// "any" type.
type DocsConfig struct {
Navigation any
Variables any
Redirects RedirectConfig
}

type RedirectConfig []Redirect

type Redirect struct {
Source string
Destination string
Permanent bool
}

// CheckDocsPathsForMissingRedirects takes the relative path to a
// gravitational/teleport clone. It assumes that there is a file called
// "docs/config.json" at the root of the directory that lists redirects in the
// "redirects" field.
//
// CheckDocsPathsForMissingRedirects checks whether a PR has renamed or deleted
// any docs files and, if so, returns an error if any of these docs files does
// not correspond to a new redirect in the configuration file.
func (b *Bot) CheckDocsPathsForMissingRedirects(ctx context.Context, teleportClonePath string) error {
if teleportClonePath == "" {
return trace.Wrap(errors.New("unable to load Teleport documentation config with an empty path"))
}

docsConfigPath := path.Join(teleportClonePath, "docs", "config.json")
f, err := os.Open(docsConfigPath)
if err != nil {
return trace.Wrap(fmt.Errorf("unable to open docs config file at %v: %v", docsConfigPath, err))
}
defer f.Close()

var c DocsConfig
if err := json.NewDecoder(f).Decode(&c); err != nil {
return trace.Wrap(fmt.Errorf("unable to load redirect configuration from %v: %v", docsConfigPath, err))
}

files, err := b.c.GitHub.ListFiles(ctx, b.c.Environment.Organization, b.c.Environment.Repository, b.c.Environment.Number)
if err != nil {
return trace.Wrap(fmt.Errorf("unable to fetch files for PR %v: %v", b.c.Environment.Number, err))
}

m := missingRedirectSources(c.Redirects, files)
if len(m) > 0 {
return trace.Wrap(fmt.Errorf("docs config at %v is missing redirects for the following renamed or deleted pages: %v", docsConfigPath, strings.Join(m, ",")))
}

return nil
}

const docsPrefix = "docs/pages/"

// toURLPath converts a local docs page path to a URL path in the format found
// in a docs redirect configuration.
func toURLPath(p string) string {
return strings.TrimSuffix(
strings.ReplaceAll(p, docsPrefix, "/"),
".mdx",
) + "/"
}

// missingRedirectSources checks renamed or deleted docs pages in files to
// ensure that there is a corresponding redirect source in conf. For any missing
// redirects, it lists redirect sources that should be in conf.
func missingRedirectSources(conf RedirectConfig, files github.PullRequestFiles) []string {
sources := make(map[string]struct{})
for _, s := range conf {
sources[s.Source] = struct{}{}
}

res := []string{}
for _, f := range files {
if !strings.HasPrefix(f.Name, docsPrefix) {
continue
}

switch f.Status {
case "renamed":
p := toURLPath(f.PreviousName)
if _, ok := sources[p]; !ok {
res = append(res, p)
}
case "removed":
p := toURLPath(f.Name)
if _, ok := sources[p]; !ok {
res = append(res, p)
}
}
}
return res
}
152 changes: 152 additions & 0 deletions bot/internal/bot/docpaths_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,152 @@
package bot

import (
"testing"

"github.com/gravitational/shared-workflows/bot/internal/github"
"github.com/stretchr/testify/assert"
)

func TestMissingRedirectSources(t *testing.T) {
cases := []struct {
description string
files github.PullRequestFiles
redirects RedirectConfig
expected []string
}{
{
description: "renamed docs path with adequate redirects",
files: github.PullRequestFiles{
{
Name: "docs/pages/databases/protect-mysql.mdx",
Additions: 0,
Deletions: 0,
Status: "renamed",
PreviousName: "docs/pages/databases/mysql.mdx",
},
},
redirects: RedirectConfig{
{
Source: "/databases/mysql/",
Destination: "/databases/protect-mysql/",
Permanent: true,
},
},
expected: []string{},
},
{
description: "renamed docs path with missing redirects",
files: github.PullRequestFiles{
{
Name: "docs/pages/databases/protect-mysql.mdx",
Additions: 0,
Deletions: 0,
Status: "renamed",
PreviousName: "docs/pages/databases/mysql.mdx",
},
},
redirects: RedirectConfig{},
expected: []string{"/databases/mysql/"},
},
{
description: "deleted docs path with adequate redirects",
files: github.PullRequestFiles{
{
Name: "docs/pages/databases/mysql.mdx",
Additions: 0,
Deletions: 0,
Status: "removed",
},
},
redirects: RedirectConfig{
{
Source: "/databases/mysql/",
Destination: "/databases/protect-mysql/",
Permanent: true,
},
}, expected: []string{},
},
{
description: "deleted docs path with missing redirects",
files: github.PullRequestFiles{
{
Name: "docs/pages/databases/mysql.mdx",
Additions: 0,
Deletions: 200,
Status: "removed",
},
},
redirects: RedirectConfig{},
expected: []string{"/databases/mysql/"},
},
{
description: "modified docs page",
files: github.PullRequestFiles{
{
Name: "docs/pages/databases/mysql.mdx",
Additions: 50,
Deletions: 15,
Status: "modified",
},
},
redirects: RedirectConfig{},
expected: []string{},
},
{
description: "renamed docs path with nil redirects",
files: github.PullRequestFiles{
{
Name: "docs/pages/databases/protect-mysql.mdx",
Additions: 0,
Deletions: 0,
Status: "renamed",
PreviousName: "docs/pages/databases/mysql.mdx",
},
},
redirects: nil,
expected: []string{"/databases/mysql/"},
},
{
description: "redirects with nil files",
files: nil,
redirects: RedirectConfig{
{
Source: "/databases/mysql/",
Destination: "/databases/protect-mysql/",
Permanent: true,
},
}, expected: []string{},
},
// TODO: Once we use the Docusaurus-native configuration syntax,
// rather than migrate the gravitational/docs configuration
// syntax, the destination will take the form,
// "/enroll-resources/databases/". I.e., the duplicate path
// segment is removed.
{
description: "renamed section index page with adequate redirects",
files: github.PullRequestFiles{
{
Name: "docs/pages/enroll-resources/databases/databases.mdx",
Additions: 0,
Deletions: 0,
Status: "renamed",
PreviousName: "docs/pages/databases/databases.mdx",
},
},
redirects: RedirectConfig{
{
Source: "/databases/databases/",
Destination: "/enroll-resources/databases/databases/",
Permanent: true,
},
},
expected: []string{},
},
}

for _, c := range cases {
t.Run(c.description, func(t *testing.T) {
assert.Equal(t, c.expected, missingRedirectSources(c.redirects, c.files))
})
}
}
13 changes: 9 additions & 4 deletions bot/internal/github/github.go
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,10 @@ type PullRequestFile struct {
Deletions int
// Status is either added, removed, modified, renamed, copied, changed, unchanged
Status string
// PreviousName is the name of the file prior to renaming. The GitHub
// API only assigns this if Status is "renamed". For deleted files, the
// GitHub API uses Name.
PreviousName string
}

// PullRequestFiles is a list of pull request files.
Expand Down Expand Up @@ -410,10 +414,11 @@ func (c *Client) ListFiles(ctx context.Context, organization string, repository

for _, file := range page {
files = append(files, PullRequestFile{
Name: file.GetFilename(),
Additions: file.GetAdditions(),
Deletions: file.GetDeletions(),
Status: file.GetStatus(),
Name: file.GetFilename(),
Additions: file.GetAdditions(),
Deletions: file.GetDeletions(),
Status: file.GetStatus(),
PreviousName: file.GetPreviousFilename(),
})
}

Expand Down
51 changes: 29 additions & 22 deletions bot/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,8 @@ func main() {
err = b.BloatCheck(ctx, flags.baseStats, flags.buildDir, flags.artifacts, os.Stdout)
case "changelog":
err = b.CheckChangelog(ctx)
case "docpaths":
err = b.CheckDocsPathsForMissingRedirects(ctx, flags.teleportClonePath)
default:
err = trace.BadParameter("unknown workflow: %v", flags.workflow)
}
Expand Down Expand Up @@ -113,21 +115,25 @@ type flags struct {
baseStats string
// buildDir is an absolute path to a directory containing build artifacts.
buildDir string
// teleportClonePath is a relative path to a gravitational/teleport
// repository clone.
teleportClonePath string
}

func parseFlags() (flags, error) {
var (
workflow = flag.String("workflow", "", "specific workflow to run [assign, check, dismiss, backport]")
token = flag.String("token", "", "GitHub authentication token")
reviewers = flag.String("reviewers", "", "reviewer assignments")
local = flag.Bool("local", false, "local workflow dry run")
org = flag.String("org", "", "GitHub organization (local mode only)")
repo = flag.String("repo", "", "GitHub repository (local mode only)")
prNumber = flag.Int("pr", 0, "GitHub pull request number (local mode only)")
branch = flag.String("branch", "", "GitHub backport branch name (local mode only)")
baseStats = flag.String("base", "", "the artifact sizes as generated by binary-sizes to compare against for bloat")
buildDir = flag.String("builddir", "", "an absolute path to a build directory containing artifacts to be checked for bloat")
artifacts = flag.String("artifacts", "", "a comma separated list of compile artifacts to analyze for bloat")
workflow = flag.String("workflow", "", "specific workflow to run [assign, check, dismiss, backport]")
token = flag.String("token", "", "GitHub authentication token")
reviewers = flag.String("reviewers", "", "reviewer assignments")
local = flag.Bool("local", false, "local workflow dry run")
org = flag.String("org", "", "GitHub organization (local mode only)")
repo = flag.String("repo", "", "GitHub repository (local mode only)")
prNumber = flag.Int("pr", 0, "GitHub pull request number (local mode only)")
branch = flag.String("branch", "", "GitHub backport branch name (local mode only)")
baseStats = flag.String("base", "", "the artifact sizes as generated by binary-sizes to compare against for bloat")
buildDir = flag.String("builddir", "", "an absolute path to a build directory containing artifacts to be checked for bloat")
artifacts = flag.String("artifacts", "", "a comma separated list of compile artifacts to analyze for bloat")
teleportClonePath = flag.String("teleport-path", "", "relative path to a gravitational/teleport clone")
)
flag.Parse()

Expand All @@ -152,17 +158,18 @@ func parseFlags() (flags, error) {
}

return flags{
workflow: *workflow,
token: *token,
reviewers: string(data),
local: *local,
org: *org,
repo: *repo,
prNumber: *prNumber,
branch: *branch,
artifacts: strings.Split(*artifacts, ","),
baseStats: string(stats),
buildDir: *buildDir,
workflow: *workflow,
token: *token,
reviewers: string(data),
local: *local,
org: *org,
repo: *repo,
prNumber: *prNumber,
branch: *branch,
artifacts: strings.Split(*artifacts, ","),
baseStats: string(stats),
buildDir: *buildDir,
teleportClonePath: *teleportClonePath,
}, nil
}

Expand Down

0 comments on commit de56c4f

Please sign in to comment.