From fcfc22a3374e503b7678a081a8750457c0366873 Mon Sep 17 00:00:00 2001 From: George Dumitrescu Date: Sat, 24 Oct 2026 10:44:04 +0300 Subject: [PATCH 1/2] ci: say in the log why a submission was refused The check wrote its findings only to the run's summary page, so a refused submission's log ended on whatever go printed last and read like a failed download. Every blocking finding is now also written as an error annotation and every note for the reviewer as a warning, which puts the reason in the log, on the pull request and in gh pr checks. --- ci/check/main.go | 53 +++++++++++++++++++++++++++++++++++-------- ci/check/main_test.go | 19 ++++++++++++++-- 2 files changed, 60 insertions(+), 12 deletions(-) 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) + } +} From ab7f7e99a5416dcd29d25c9c49c35cf00f3640e9 Mon Sep 17 00:00:00 2001 From: George Dumitrescu Date: Sat, 24 Oct 2026 10:44:04 +0300 Subject: [PATCH 2/2] ci: check each area of a submission in a step of its own The check runs once per area, the entry itself, the repository, the manifest, services, images, code and installing it beside the listed plugins, each as its own step. The step that goes red names what is wrong, and an entry that cannot be fetched skips the rest instead of repeating the same error in each. --- .github/workflows/check.yml | 47 ++++++++++++++++++++++++++++++++++--- ci/check/check_test.go | 18 ++++++++++++++ ci/check/inspect.go | 41 +++++++++++++++++++++++--------- 3 files changed, 92 insertions(+), 14 deletions(-) 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+)?$`)