diff --git a/.github/workflows/check.yml b/.github/workflows/check.yml index 83f6bc7..087f62e 100644 --- a/.github/workflows/check.yml +++ b/.github/workflows/check.yml @@ -70,16 +70,57 @@ jobs: go build -tags nogui -ldflags "-X github.com/geodro/lerd/internal/version.Version=dev" -o "$RUNNER_TEMP/lerd/lerd" ./cmd/lerd "$RUNNER_TEMP/lerd/lerd" --version - - name: Check the changed entries + # Each area is a step of its own, so the step that fails names what is wrong + # and its log says why; the summary page carries the whole report. + - name: "Entry: the file, a public repository, a commit that fetches" + id: entry env: GITHUB_TOKEN: ${{ github.token }} run: | - changed=$(git diff --name-only --diff-filter=AM "origin/${{ github.base_ref }}...HEAD" -- 'plugins/*.yaml') + changed=$(git diff --name-only --diff-filter=AM "origin/${{ github.base_ref }}...HEAD" -- 'plugins/*.yaml' | tr '\n' ' ') + echo "changed=$changed" >> "$GITHUB_OUTPUT" if [ -z "$changed" ]; then echo "No entry added or changed." >> "$GITHUB_STEP_SUMMARY" exit 0 fi - go run ./ci/check -lerd "$RUNNER_TEMP/lerd/lerd" -report "$GITHUB_STEP_SUMMARY" $changed + go build -o check-bin ./ci/check + ./check-bin -lerd "$RUNNER_TEMP/lerd/lerd" -only entry -report "$GITHUB_STEP_SUMMARY" $changed + + - name: "Repository: a license and a README" + if: ${{ !cancelled() && steps.entry.outcome == 'success' && steps.entry.outputs.changed }} + env: + GITHUB_TOKEN: ${{ github.token }} + run: ./check-bin -lerd "$RUNNER_TEMP/lerd/lerd" -only repository -report "$GITHUB_STEP_SUMMARY" ${{ steps.entry.outputs.changed }} + + - name: "Manifest: lerd bound, platforms, screenshots" + if: ${{ !cancelled() && steps.entry.outcome == 'success' && steps.entry.outputs.changed }} + env: + GITHUB_TOKEN: ${{ github.token }} + run: ./check-bin -lerd "$RUNNER_TEMP/lerd/lerd" -only manifest -report "$GITHUB_STEP_SUMMARY" ${{ steps.entry.outputs.changed }} + + - name: "Services: pinned images the README names" + if: ${{ !cancelled() && steps.entry.outcome == 'success' && steps.entry.outputs.changed }} + env: + GITHUB_TOKEN: ${{ github.token }} + run: ./check-bin -lerd "$RUNNER_TEMP/lerd/lerd" -only services -report "$GITHUB_STEP_SUMMARY" ${{ steps.entry.outputs.changed }} + + - name: "Images: format and size" + if: ${{ !cancelled() && steps.entry.outcome == 'success' && steps.entry.outputs.changed }} + env: + GITHUB_TOKEN: ${{ github.token }} + run: ./check-bin -lerd "$RUNNER_TEMP/lerd/lerd" -only images -report "$GITHUB_STEP_SUMMARY" ${{ steps.entry.outputs.changed }} + + - name: "Code: no binaries, nothing piped to a shell" + if: ${{ !cancelled() && steps.entry.outcome == 'success' && steps.entry.outputs.changed }} + env: + GITHUB_TOKEN: ${{ github.token }} + run: ./check-bin -lerd "$RUNNER_TEMP/lerd/lerd" -only code -report "$GITHUB_STEP_SUMMARY" ${{ steps.entry.outputs.changed }} + + - name: "Installs beside every listed plugin" + if: ${{ !cancelled() && steps.entry.outcome == 'success' && steps.entry.outputs.changed }} + env: + GITHUB_TOKEN: ${{ github.token }} + run: ./check-bin -lerd "$RUNNER_TEMP/lerd/lerd" -only install -report "$GITHUB_STEP_SUMMARY" ${{ steps.entry.outputs.changed }} # A submission changes its own entry and nothing else; changes to the checks # come from maintainers in pull requests of their own. diff --git a/ci/check/check_test.go b/ci/check/check_test.go index 570c4d7..c0892be 100644 --- a/ci/check/check_test.go +++ b/ci/check/check_test.go @@ -197,3 +197,21 @@ func TestInstallableHere(t *testing.T) { t.Errorf("the copy carries every file: %v", err) } } + +// CI runs each area as a step of its own, so an area reports only what it +// judges: the step that goes red then names what is wrong. +func TestInspectArea_ReportsOnlyItsOwnFindings(t *testing.T) { + dir := plugin(t, map[string]string{"bin/tool": "\x7fELF\x00\x00binary"}) + if err := os.Remove(filepath.Join(dir, "LICENSE")); err != nil { + t.Fatal(err) + } + var code, repo report + inspectArea(&code, dir, "qb", registry.Entry{}, "code") + inspectArea(&repo, dir, "qb", registry.Entry{}, "repository") + if !has(code.Fail, "bin/tool") || has(code.Fail, "LICENSE") { + t.Errorf("code found %v, want only the binary", code.Fail) + } + if !has(repo.Fail, "LICENSE") || has(repo.Fail, "bin/tool") { + t.Errorf("repository found %v, want only the license", repo.Fail) + } +} diff --git a/ci/check/inspect.go b/ci/check/inspect.go index ae25537..12b8264 100644 --- a/ci/check/inspect.go +++ b/ci/check/inspect.go @@ -24,23 +24,42 @@ type report struct { func (r *report) fail(format string, a ...any) { r.Fail = append(r.Fail, fmt.Sprintf(format, a...)) } func (r *report) flag(format string, a ...any) { r.Flag = append(r.Flag, fmt.Sprintf(format, a...)) } -// inspect checks a plugin checked out at its approved commit in dir. +// areas are the parts of a submission the check judges. CI runs each as a step +// of its own, so the step that fails names what is wrong. +var areas = []string{"repository", "manifest", "services", "images", "code", "install"} + +// inspect checks a plugin checked out at its approved commit in dir, every +// area but installing it. func inspect(dir, name string, e registry.Entry) report { var r report - if !anyFile(dir, "LICENSE", "LICENSE.md", "LICENSE.txt", "COPYING") { - r.fail("no LICENSE file (rule 2)") - } - readme, ok := readFirst(dir, "README.md", "README", "readme.md") - if !ok { - r.fail("no README (rule 3)") + for _, a := range areas { + inspectArea(&r, dir, name, e, a) } - checkManifest(&r, dir, name, len(e.Images) > 0) - checkServices(&r, dir, readme) - checkImages(&r, dir, e.Images) - scanFiles(&r, dir) return r } +// inspectArea checks one area of the plugin; install is checkEntry's, not this. +func inspectArea(r *report, dir, name string, e registry.Entry, area string) { + readme, hasReadme := readFirst(dir, "README.md", "README", "readme.md") + switch area { + case "repository": + if !anyFile(dir, "LICENSE", "LICENSE.md", "LICENSE.txt", "COPYING") { + r.fail("no LICENSE file (rule 2)") + } + if !hasReadme { + r.fail("no README (rule 3)") + } + case "manifest": + checkManifest(r, dir, name, len(e.Images) > 0) + case "services": + checkServices(r, dir, readme) + case "images": + checkImages(r, dir, e.Images) + case "code": + scanFiles(r, dir) + } +} + // lowerBoundRe accepts only a lower bound, so a new lerd never strands the plugin. var lowerBoundRe = regexp.MustCompile(`^>=\s*\d+\.\d+(\.\d+)?$`) diff --git a/ci/check/main.go b/ci/check/main.go index 547db54..e4addf5 100644 --- a/ci/check/main.go +++ b/ci/check/main.go @@ -14,6 +14,7 @@ import ( "os" "os/exec" "path/filepath" + "slices" "strings" ) @@ -21,9 +22,14 @@ func main() { lerd := flag.String("lerd", "", "lerd binary that validates and installs each plugin") plugins := flag.String("plugins", "plugins", "folder holding every listed entry") out := flag.String("report", "", "file the markdown report is appended to (stdout when empty)") + only := flag.String("only", "", "check one area: entry, "+strings.Join(areas, ", ")) flag.Parse() if *lerd == "" || flag.NArg() == 0 { - fmt.Fprintln(os.Stderr, "usage: check -lerd plugins/.yaml...") + fmt.Fprintln(os.Stderr, "usage: check -lerd [-only ] plugins/.yaml...") + os.Exit(2) + } + if *only != "" && *only != "entry" && !slices.Contains(areas, *only) { + fmt.Fprintf(os.Stderr, "unknown area %q, want entry, %s\n", *only, strings.Join(areas, ", ")) os.Exit(2) } @@ -40,8 +46,9 @@ func main() { failed := false for _, path := range flag.Args() { - name, r := checkEntry(path, *plugins, *lerd) - writeReport(w, name, r) + name, r := checkEntry(path, *plugins, *lerd, *only) + writeReport(w, name, *only, r) + annotate(os.Stdout, name, r) failed = failed || len(r.Fail) > 0 } if failed { @@ -49,7 +56,10 @@ func main() { } } -func checkEntry(path, plugins, lerd string) (string, report) { +// checkEntry checks one entry; only names a single area, or "" for all of them. +// The entry itself, the repository being public and the commit fetching are +// checked whatever the area, since nothing else can be judged without them. +func checkEntry(path, plugins, lerd, only string) (string, report) { var r report name, e, err := registry.ReadEntry(path) if err != nil { @@ -71,10 +81,18 @@ func checkEntry(path, plugins, lerd string) (string, report) { } defer os.RemoveAll(dir) - found := inspect(dir, name, e) - r.Fail = append(r.Fail, found.Fail...) - r.Flag = append(r.Flag, found.Flag...) - installAlongside(&r, lerd, plugins, name, dir) + switch only { + case "": + found := inspect(dir, name, e) + r.Fail = append(r.Fail, found.Fail...) + r.Flag = append(r.Flag, found.Flag...) + installAlongside(&r, lerd, plugins, name, dir) + case "install": + installAlongside(&r, lerd, plugins, name, dir) + case "entry": + default: + inspectArea(&r, dir, name, e, only) + } return name, r } @@ -180,7 +198,22 @@ func lerdInstall(lerd string, env []string, dir string) (string, error) { return strings.TrimSpace(string(out)), err } -func writeReport(w io.Writer, name string, r report) { +// annotate writes each finding as a GitHub workflow command, so it shows in the +// job's log and on the pull request rather than only on the summary page. +func annotate(w io.Writer, name string, r report) { + esc := strings.NewReplacer("%", "%25", "\r", "%0D", "\n", "%0A") + for _, f := range r.Fail { + fmt.Fprintf(w, "::error title=%s::%s\n", name, esc.Replace(f)) + } + for _, f := range r.Flag { + fmt.Fprintf(w, "::warning title=%s::%s\n", name, esc.Replace(f)) + } +} + +func writeReport(w io.Writer, name, only string, r report) { + if only != "" { + name += ": " + only + } fmt.Fprintf(w, "## %s\n\n", name) if len(r.Fail) > 0 { fmt.Fprintln(w, "**Blocking**") @@ -198,7 +231,7 @@ func writeReport(w io.Writer, name string, r report) { } fmt.Fprintln(w) } - if len(r.Fail) == 0 { + if len(r.Fail) == 0 && (only == "" || only == "install") { fmt.Fprintln(w, "The automated checks pass. A maintainer still reads the code for rules 9 to 15 and 19 to 23 before approving.") fmt.Fprintln(w) } diff --git a/ci/check/main_test.go b/ci/check/main_test.go index d0704b9..db6172c 100644 --- a/ci/check/main_test.go +++ b/ci/check/main_test.go @@ -62,13 +62,28 @@ func TestCheckEntry_InstallsAlongsideTheListedPlugins(t *testing.T) { registry.RepoPattern = regexpAny t.Cleanup(func() { registry.RepoPattern = origRepo }) - if _, r := checkEntry(entryFor("fresh", "jobbox"), plugins, lerd); len(r.Fail) != 0 || len(r.Flag) != 0 { + if _, r := checkEntry(entryFor("fresh", "jobbox"), plugins, lerd, ""); len(r.Fail) != 0 || len(r.Flag) != 0 { t.Errorf("a plugin with names of its own passes, got %+v", r) } - _, r := checkEntry(entryFor("copycat", "queuebox"), plugins, lerd) + _, r := checkEntry(entryFor("copycat", "queuebox"), plugins, lerd, "") if !has(r.Fail, "queuebox") { t.Errorf("a service another listed plugin ships must block, got %+v", r) } } var regexpAny = regexp.MustCompile(`.`) + +// The summary page is where the report reads best, but a failed job's log has +// to say why too, or it ends on whatever go printed last and looks broken. +func TestAnnotate_PutsEveryFindingInTheLog(t *testing.T) { + var b strings.Builder + annotate(&b, "site-services", report{ + Fail: []string{"bin/x.pyc: binary file (rule 5)"}, + Flag: []string{"bin/run:3: sudo\nnext line, 100%"}, + }) + want := "::error title=site-services::bin/x.pyc: binary file (rule 5)\n" + + "::warning title=site-services::bin/run:3: sudo%0Anext line, 100%25\n" + if b.String() != want { + t.Errorf("annotate wrote\n%q\nwant\n%q", b.String(), want) + } +}