diff --git a/bot/internal/bot/docpaths.go b/bot/internal/bot/docpaths.go new file mode 100644 index 00000000..071b4459 --- /dev/null +++ b/bot/internal/bot/docpaths.go @@ -0,0 +1,122 @@ +package bot + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "os" + "path" + "path/filepath" + "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 { + d, f := filepath.Split(p) + + // The page is a category index page, since the name of the file is the + // same as the name of the containing directory. This means that the + // route Docusaurus generates for the page is the path to the containing + // directory, omitting the final path segment. + // See: + // https://docusaurus.io/docs/sidebar/autogenerated#category-index-convention + if strings.HasSuffix(d, strings.Replace(f, ".mdx", "/", 1)) { + p = d + } + return strings.Replace( + strings.Replace(p, docsPrefix, "/", 1), + ".mdx", "/", 1) +} + +// 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 +} diff --git a/bot/internal/bot/docpaths_test.go b/bot/internal/bot/docpaths_test.go new file mode 100644 index 00000000..fb5b31a0 --- /dev/null +++ b/bot/internal/bot/docpaths_test.go @@ -0,0 +1,191 @@ +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{}, + }, + // Accommodate the Docusaurus logic of generating a route for a + // section index page, e.g., docs/pages/slug/slug.mdx, at a URL + // path that consists only of the containing directory, e.g., + // /docs/slug/. + { + 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/", + Destination: "/enroll-resources/databases/", + Permanent: true, + }, + }, + expected: []string{}, + }, + { + description: "renamed section index page with inadequate redirects", + files: github.PullRequestFiles{ + { + Name: "docs/pages/enroll-resources/applications/applications.mdx", + Additions: 0, + Deletions: 0, + Status: "renamed", + PreviousName: "docs/pages/applications/applications.mdx", + }, + }, + redirects: RedirectConfig{ + { + Source: "/applications/", + Destination: "/enroll-resources/applications/applications/", + 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)) + }) + } +} + +func Test_toURLPath(t *testing.T) { + cases := []struct { + description string + input string + expected string + }{ + { + description: "section index page", + input: "docs/pages/databases/databases.mdx", + expected: "/databases/", + }, + } + + for _, c := range cases { + t.Run(c.description, func(t *testing.T) { + assert.Equal(t, c.expected, toURLPath(c.input)) + }) + } +} diff --git a/bot/internal/github/github.go b/bot/internal/github/github.go index 7cd1e737..b9eacc4c 100644 --- a/bot/internal/github/github.go +++ b/bot/internal/github/github.go @@ -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. @@ -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(), }) } diff --git a/bot/main.go b/bot/main.go index 6ae2fc07..7d97a884 100644 --- a/bot/main.go +++ b/bot/main.go @@ -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) } @@ -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() @@ -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 }