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

Add support for sub-indexes #120

Merged
merged 5 commits into from
Oct 18, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/go.yml
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ jobs:
- name: Setup Go
uses: actions/setup-go@v4
with:
go-version: 1.20.x
go-version: 1.21.x
cache: true

- name: Lint
Expand Down
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,11 @@ All notable changes to this project will be documented in this file.
The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## Unreleased
### Added
- Generate a package listing for sub-paths
that match a subset of the known packages.

## [1.4.0]
### Added
- Publish a Docker image to GitHub Container Registry.
Expand Down
2 changes: 1 addition & 1 deletion Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
# It does not include a sally configuration.
# A /sally.yaml file is required for this to run.

FROM golang:1.19-alpine
FROM golang:1.21-alpine

COPY . /build
WORKDIR /build
Expand Down
4 changes: 3 additions & 1 deletion go.mod
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
module go.uber.org/sally

go 1.18
go 1.21

toolchain go1.21.3

require (
github.com/stretchr/testify v1.8.4
Expand Down
209 changes: 134 additions & 75 deletions handler.go
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
package main

import (
"cmp"
"fmt"
"html/template"
"net/http"
"sort"
"path"
"slices"
"strings"

"go.uber.org/sally/templates"
Expand All @@ -17,112 +19,169 @@
template.New("package.html").Parse(templates.Package))
)

// CreateHandler creates a Sally http.Handler
// CreateHandler builds a new handler
// with the provided package configuration.
// The returned handler provides the following endpoints:
//
// GET /
// Index page listing all packages.
// GET /<name>
// Package page for the given package.
// GET /<dir>
// Page listing packages under the given directory,
// assuming that there's no package with the given name.
// GET /<name>/<subpkg>
// Package page for the given subpackage.
func CreateHandler(config *Config) http.Handler {
mux := http.NewServeMux()

pkgs := make([]packageInfo, 0, len(config.Packages))
pkgs := make([]*sallyPackage, 0, len(config.Packages))
Comment on lines 36 to +37
Copy link
Member

Choose a reason for hiding this comment

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

nit: var group

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree with var grouping when var form is used, but I think when we're initializing with non-zero values, var form is rarely used.

Per the style guide, we use var form for zero value initialization and := form for non-zero-value:
https://github.com/uber-go/guide/blob/master/style.md#use-var-for-zero-value-structs
https://github.com/uber-go/guide/blob/master/style.md#initializing-struct-references

(That links only talk about structs, but the guidance has been consistent on this.)

The readability in a var group suffers as soon as you add a type for any actual var-form-zero-values.

var (
  mux           = http.NewServeMux()
  pkgs          = make(...)
  foo io.Reader
)

Copy link
Member

Choose a reason for hiding this comment

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

It's fine as-is. :) Not trying to bikeshed.

My point was more that I don't personally see the value in segmenting the initializations here so I would probably group them all. I disagree about var group readability suffering when mixing initialization and zero-value declarations (and I think that ultimately leads to a lot more ad hoc initialization/declaration patterns).

for name, pkg := range config.Packages {
handler := newPackageHandler(config, name, pkg)
baseURL := config.URL
if pkg.URL != "" {
// Package-specific override for the base URL.
baseURL = pkg.URL
}
modulePath := path.Join(baseURL, name)
docURL := "https://" + path.Join(config.Godoc.Host, modulePath)

pkg := &sallyPackage{
Name: name,
Desc: pkg.Desc,
ModulePath: modulePath,
DocURL: docURL,
GitURL: pkg.Repo,
}
Comment on lines +44 to +53
Copy link
Member

Choose a reason for hiding this comment

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

nit: grouping

Suggested change
modulePath := path.Join(baseURL, name)
docURL := "https://" + path.Join(config.Godoc.Host, modulePath)
pkg := &sallyPackage{
Name: name,
Desc: pkg.Desc,
ModulePath: modulePath,
DocURL: docURL,
GitURL: pkg.Repo,
}
var (
modulePath = path.Join(baseURL, name)
docURL = "https://" + path.Join(config.Godoc.Host, modulePath)
pkg = &sallyPackage{
Name: name,
Desc: pkg.Desc,
ModulePath: modulePath,
DocURL: docURL,
GitURL: pkg.Repo,
}
)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same comment as above. The var form is just less readable here. := for non-zero value initialization has always been the way.

pkgs = append(pkgs, pkg)

// Double-register so that "/foo"
// does not redirect to "/foo/" with a 300.
handler := &packageHandler{Pkg: pkg}
mux.Handle("/"+name, handler)
mux.Handle("/"+name+"/", handler)

pkgs = append(pkgs, packageInfo{
Desc: pkg.Desc,
ImportPath: handler.canonicalURL,
GitURL: handler.gitURL,
GodocHome: handler.godocHost + "/" + handler.canonicalURL,
})
}
sort.Slice(pkgs, func(i, j int) bool {
return pkgs[i].ImportPath < pkgs[j].ImportPath

mux.Handle("/", newIndexHandler(pkgs))
return requireMethod(http.MethodGet, mux)
}

func requireMethod(method string, handler http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if r.Method != method {
http.NotFound(w, r)
return
}

handler.ServeHTTP(w, r)
})
mux.Handle("/", &indexHandler{pkgs: pkgs})
}

return mux
type sallyPackage struct {
// Name of the package.
//
// This is the part after the base URL.
Name string

// Canonical import path for the package.
ModulePath string

// Description of the package, if any.
Desc string

// URL at which documentation for the package can be found.
DocURL string

// URL at which the Git repository is hosted.
GitURL string
}

type indexHandler struct {
pkgs []packageInfo
pkgs []*sallyPackage // sorted by name
}

type packageInfo struct {
Desc string // package description
ImportPath string // canonical import path
GitURL string // URL of the Git repository
GodocHome string // documentation home URL
}
var _ http.Handler = (*indexHandler)(nil)

func (h *indexHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
// Index handler only supports '/'.
// ServeMux will call us for any '/foo' that is not a known package.
if r.Method != http.MethodGet || r.URL.Path != "/" {
http.NotFound(w, r)
return
}
func newIndexHandler(pkgs []*sallyPackage) *indexHandler {
slices.SortFunc(pkgs, func(a, b *sallyPackage) int {
return cmp.Compare(a.Name, b.Name)
})

data := struct{ Packages []packageInfo }{
Packages: h.pkgs,
}
if err := indexTemplate.Execute(w, data); err != nil {
http.Error(w, err.Error(), 500)
return &indexHandler{
pkgs: pkgs,
}
}

type packageHandler struct {
// Hostname of the godoc server, e.g. "godoc.org".
godocHost string
func (h *indexHandler) rangeOf(path string) (start, end int) {
if len(path) == 0 {
return 0, len(h.pkgs)
}

// Name of the package relative to the vanity base URL.
// For example, "zap" for "go.uber.org/zap".
name string
// If the packages are sorted by name,
// we can scan adjacent packages to find the range of packages
// whose name descends from path.
start, _ = slices.BinarySearchFunc(h.pkgs, path, func(pkg *sallyPackage, path string) int {
return cmp.Compare(pkg.Name, path)
})

// Path at which the Git repository is hosted.
// For example, "github.com/uber-go/zap".
gitURL string
for idx := start; idx < len(h.pkgs); idx++ {
if !descends(path, h.pkgs[idx].Name) {
// End of matching sequences.
// The next path is not a descendant of path.
return start, idx
}
}

// Canonical import path for the package.
canonicalURL string
// All packages following start are descendants of path.
// Return the rest of the packages.
return start, len(h.pkgs)
}

func newPackageHandler(cfg *Config, name string, pkg PackageConfig) *packageHandler {
baseURL := cfg.URL
if pkg.URL != "" {
baseURL = pkg.URL
func (h *indexHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
path := strings.TrimPrefix(strings.TrimSuffix(r.URL.Path, "/"), "/")
start, end := h.rangeOf(path)

// If start == end, then there are no packages
if start == end {
w.WriteHeader(http.StatusNotFound)
fmt.Fprintf(w, "no packages found under path: %v\n", path)
return
}
canonicalURL := fmt.Sprintf("%s/%s", baseURL, name)

return &packageHandler{
godocHost: cfg.Godoc.Host,
name: name,
canonicalURL: canonicalURL,
gitURL: pkg.Repo,
err := indexTemplate.Execute(w,
struct{ Packages []*sallyPackage }{
Packages: h.pkgs[start:end],
})
if err != nil {
http.Error(w, err.Error(), 500)

Check warning on line 154 in handler.go

View check run for this annotation

Codecov / codecov/patch

handler.go#L154

Added line #L154 was not covered by tests
}
}

func (h *packageHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
if r.Method != http.MethodGet {
http.NotFound(w, r)
return
}
type packageHandler struct {
Pkg *sallyPackage
}

var _ http.Handler = (*packageHandler)(nil)

func (h *packageHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
// Extract the relative path to subpackages, if any.
// "/foo/bar" => "/bar"
// "/foo" => ""
relPath := strings.TrimPrefix(r.URL.Path, "/"+h.name)

data := struct {
Repo string
CanonicalURL string
GodocURL string
}{
Repo: h.gitURL,
CanonicalURL: h.canonicalURL,
GodocURL: fmt.Sprintf("https://%s/%s%s", h.godocHost, h.canonicalURL, relPath),
}
if err := packageTemplate.Execute(w, data); err != nil {
// "/foo/bar" => "/bar"
// "/foo" => ""
relPath := strings.TrimPrefix(r.URL.Path, "/"+h.Pkg.Name)

err := packageTemplate.Execute(w,
struct {
ModulePath string
GitURL string
DocURL string
}{
ModulePath: h.Pkg.ModulePath,
GitURL: h.Pkg.GitURL,
DocURL: h.Pkg.DocURL + relPath,
})
abhinav marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
http.Error(w, err.Error(), 500)
}
}

func descends(from, to string) bool {
return to == from || (strings.HasPrefix(to, from) && to[len(from)] == '/')
}
Loading