diff --git a/doc/package-spec.md b/doc/package-spec.md new file mode 100644 index 00000000..ff4748f5 --- /dev/null +++ b/doc/package-spec.md @@ -0,0 +1,151 @@ +# DAppNode package spec + +This spec narrows what a DAppNode package repository may contain. Many repositories rely on behaviour that the SDK, DAPPMANAGER and tropibot only tolerate: legacy fields, alternative file names, values that are rewritten at install, and layouts that need guessing. Each tolerated variation costs code in every consumer, and some of it silently produces broken or incomplete releases. + +The SDK checks every package against these rules during `build` and `publish`. + +## Rollout + +| Phase | SDK behaviour | Who acts | +|---|---|---| +| 1. Warn (this release) | Violations are printed after the run, with rule id, file and fix. In GitHub Actions they also appear as `::warning` annotations on the run and the pull request. Builds never fail because of the spec. | Package maintainers fix warnings in normal PRs. | +| 2. Opt-in strict | `DAPPNODE_SDK_SPEC=error` makes validation fail on any violation. Set it in `dappnode/workflows` for repositories that are already clean, or for new packages. | Workflow owners and tropibot. | +| 3. Strict by default | `error` becomes the default; `DAPPNODE_SDK_SPEC=warn` remains as a temporary escape hatch. | SDK release, announced in advance. | +| 4. Remove tolerances | The SDK and tropibot drop the code paths listed under each rule. | SDK and tropibot maintainers. | + +`DAPPNODE_SDK_SPEC=off` disables the checks. Rule ids are stable: a rule keeps its meaning when it moves from warning to error. + +## Rules + +Counts are from all 113 non-archived `DAppNodePackage*` and `DNP_*` repositories in November 2026; "active" means a push in the last 12 months (87 repositories). 58 active repositories currently have at least one finding, mostly SPEC-M03; 37 repositories are already clean. + +### Manifest + +**SPEC-M01: One manifest named `dappnode_package.json`.** Each package directory (root and every variant) contains exactly one manifest, in JSON, named `dappnode_package.json`. `sdk-publish`, the ISO installer and tropibot read that exact name, and when several files match `dappnode_package*` the SDK picks one arbitrarily. Today: 0 active repositories. + +**SPEC-M02: Package name format.** `name` matches `^[a-z0-9]+(-[a-z0-9]+)*\.(dnp|public)\.dappnode\.eth$`. Today: 2 inactive (Zosma-Nethermind, Zosma-OE). + +**SPEC-M03: Upstream declared with `upstream[]`.** Upstream software is declared as `"upstream": [{ "repo", "version", "arg" }]`. The legacy `upstreamRepo`, `upstreamVersion` and `upstreamArg` fields are deprecated. Today: 44 active repositories still use them, including DNP_BIND, DNP_IPFS, Hopr, NEAR, Hermes-agent and Honcho. This is the largest migration and removes a second code path from the SDK bumper, tropibot's discovery and edit code, and the dashboards. + +**SPEC-M04: The upstream `arg` is a compose build arg.** Every upstream entry names an `arg` that some compose service declares in `build.args`. Packages using the legacy fields often rely on the implicit default `UPSTREAM_VERSION`. Today: 9 active (SafeStake-testnet, SplitPro, avalanche, graph, monero, rocketpool, rocketpool-testnet, supabase, web3signer-kiln). + +**SPEC-M05: The upstream `version` equals the build arg value.** The manifest version and the compose build arg are the same string, so tools never translate between `v1.2.3` and `1.2.3`. A digest-pinned arg (`1.2.3@sha256:…`) counts as equal. Today: 4 active (GnosisVPNExitNode, Hopr-testnet, Tornado-Cash-Relayer, aztec-testnet). + +**SPEC-M06: A variant `upstream[]` keeps the root's positions.** Root and variant manifests are merged by array position. A variant may list fewer or more entries than the root, but an entry at an existing position must refer to the same `repo`. Today: 0 repositories. + +**SPEC-M07: Only fields from the manifest schema.** Fields outside the published manifest schema (plus `upnpDisable`, supported by DAPPMANAGER but not yet published) are ignored or stripped at install. Today: 1 active (owncloud: `minimumDappnodeVersion`), 5 inactive with legacy `avatar`/`image`. + +**SPEC-M08: `repository.url` points to the package repository.** When a manifest declares `repository`, its URL is the package's own GitHub repository (`https://` or `git+https://`, with or without `.git`). Today: 8 active point elsewhere: renamed repositories (DMS, anchor-generic), sibling packages (Erigon-Sepolia-Archival, lukso-erigon, NEAR-testnet), a fork (avalanche) and upstream projects (SplitPro, supabase). + +**SPEC-M09: Exposable services exist in the compose.** `exposable[].serviceName` names a service in the compose. Today: 2 active (op-erigon, op-geth). + +### Templates (multi-variant packages) + +**SPEC-T01: The template root manifest has no `name` or `version`.** When a package has variants, each variant manifest declares its own `name` and `version` and the root holds only shared fields. A named root becomes a phantom package for pin cleanup, dashboards and on-chain sync. Today: 5 active (juno-generic, jupyter-generic, nethermind-generic, ollama-generic, pathfinder-generic) and madara-generic. + +**SPEC-T02: Variants live in `package_variants/`.** A custom `--variants_dir_name` is reported. Workflows, tropibot and the staking tests only read `package_variants/`. Today: 0 repositories. + +**SPEC-T03: Variant directory names.** Variant names match `^[a-z0-9]+(-[a-z0-9]+)*$`; they appear in git tags (`@v`) and ENS names. Today: 0 repositories. + +**SPEC-T04: A single release file is in the root or the variant, not both.** For avatar, Prometheus targets, disclaimer, getting-started and the legacy setup files, the variant copy replaces the root copy, so a root copy next to variant copies is dead and goes stale. Setup wizards and notifications are merged by design, and Grafana dashboards are collected from both, so they are exempt. Today: 1 active (erigon-generic: a stale root `prometheus-targets.json`). + +**SPEC-T05: Every variant directory has a manifest.** Today: 0 repositories. + +### Compose + +**SPEC-C01: The compose file is `docker-compose.yml`.** A custom `--compose_file_name` is reported. DAPPMANAGER only recognises `compose*.yml` in a release, so `docker-compose.yaml` or `production.yml` produce a release that cannot be installed. Today: 0 repositories use the flag. + +**SPEC-C02: No `container_name`.** DAPPMANAGER replaces it at install, so it has no effect. Core packages (`type: dncore`) are exempt: the installer starts them directly and addresses them by name. Today: 6 active (Hermes-agent, Nextcloud, graph, jupyter-generic, ollama-generic, openclaw). + +**SPEC-C03: External images are pinned.** A service without `build` uses an image with an explicit, non-`latest` tag or a digest. Today: 2 active (graph: `postgres`; owncloud: `webhippie/mariadb:latest`, `webhippie/redis:latest`). + +**SPEC-C04: No `${VAR}` interpolation.** Compose values are written out; interpolation depends on the environment of whoever runs compose. Today: 2 active (alephium, supabase) and madara-generic. + +**SPEC-C05: `build.args` is a mapping.** `args: { KEY: value }`, not a list of `KEY=value` strings, so every tool that edits upstream versions needs one parser. Today: 3 active (DNP_NOTIFICATIONS, pi-hole, portainer). + +### Release files + +**SPEC-F01: Release files use the recognised names.** A file that looks like a release file but does not match its pattern is silently left out of the release. Today: 5 active: NEAR `setup-wizard-ui.yml`, Tornado-Cash-Relayer `avatar-default.jpg`, package-harness `avatar-harness.svg`, pathfinder-generic `getting-started.yml` (must be `.md`) and reth-gnosis `notifications.yml` (must be `.yaml`). + +| Type | Recognised names | +|---|---| +| Manifest | `dappnode_package.json` | +| Compose | `docker-compose.yml` | +| Avatar | `avatar*.png` | +| Setup wizard | `setup-wizard.json`, `setup-wizard.yml`, `setup-wizard.yaml` | +| Notifications | `notifications.yaml` | +| Prometheus targets | `prometheus-targets.json` (one file) | +| Grafana dashboards | `*grafana-dashboard.json` (any number) | +| Disclaimer | `disclaimer.md` | +| Getting started | `getting-started.md` | + +**SPEC-F02: Release files stay within the size limits** from `@dappnode/types` `releaseFiles` (avatar 100 KB, compose 10 KB, setup wizard 100 KB, notifications 10 KB, Prometheus targets 10 KB, dashboards 10 MB). Today: 2 active (Lido-CSM-generic avatar 123 KB, lukso-erigon avatar 245 KB). + +**SPEC-F03: Values already have the schema type.** Validation coerces values in place (for example `isBanner: "false"` becomes `false`), and the release ships the coerced value. Today: 1 active (DMS notifications). + +### Build inputs + +**SPEC-B01: Compose validation is not skipped.** DAPPMANAGER validates the compose again at install, so `--skip_compose_validation` only defers the failure to users. Today: no workflow uses it. + +**SPEC-B02: No `UPSTREAM_VERSION` environment override.** The upstream version comes from the package files, never from the build environment. + +## Proposed for a later phase + +These are not checked yet, either because they need consumer changes first or because they are too noisy today. + +- **Compose `version:`** is obsolete (109 repositories still set it). Rather than warn, the SDK should strip it from releases by default once versionless composes are verified with DAPPMANAGER (`--strip_compose_version` exists today as an opt-in). +- **Dockerfiles:** no `FROM …:latest` or untagged base images (6 inactive repositories), and no remote `ADD` without `--checksum` (20 active repositories; 19 are the staking `ADD ${STAKER_SCRIPTS_URL}/*.sh` pattern, which needs a shared replacement first). +- **Declarative upstream metadata:** optional `upstream[].provider`, `tagPattern` and `imageTag` fields would let tropibot drop its built-in provider table and tag-to-image-tag inference. This needs a manifest schema change. +- **ENS names of new variants:** `-..dappnode.eth` for new variants, with an allowlist of existing on-chain names (`hoodi-geth`, `nethermind-xdai`, …). +- **Explicit `architectures`** (25 repositories rely on the implicit `linux/amd64`). +- **Setup wizard targets:** every wizard target refers to a service and environment variable that exist in the compose (7 repositories reference variables the compose does not declare). +- **One next-version rule** shared by the SDK (APM) and tropibot (DApp Explorer +1 patch). + +## Consumer contradictions found + +These came up while defining the rules and should be fixed regardless of the rollout. + +- A custom `--compose_file_name` yields a release DAPPMANAGER cannot install (SPEC-C01). +- A YAML manifest is released as `dappnode_package.yml`, which `sdk-publish` and the ISO installer cannot read (SPEC-M01). +- The SDK reads notifications with any extension, DAPPMANAGER only `.yaml` (SPEC-F01). +- `globalEnvs: { all: true }` is documented and implemented in DAPPMANAGER but rejected by the schema; the docs should drop it. +- The manifest schema's `version` pattern has unescaped dots and `name` has no pattern; the SDK checks both itself (`validatePackageVersion`, SPEC-M02). +- The variants directory default is `variants` in the bump workflow and in tropibot's package creation guide, but `package_variants` everywhere else (SPEC-T02). + +## tropibot + +tropibot and the SDK share most package handling. Once a rule is an error, tropibot can drop the matching tolerance: + +| Rule | tropibot code that becomes removable | +|---|---| +| SPEC-M01, SPEC-C01 | Hard-coded name assumptions in discovery, pin cleanup, on-chain sync and dashboard artifacts become guarantees. | +| SPEC-M03 | The legacy `upstreamRepo/Version/Arg` path in bump discovery and edits (`bump/package/discovery.rs`, `edits.rs`); dashboards keep it only for history. | +| SPEC-M04, SPEC-C05 | Upstream arg detection heuristics and the `UPSTREAM_VERSION` default (`discovery.rs`, `compose.rs`), list-style arg parsing and string `build:` handling. | +| SPEC-M05 | Part of the tag/arg format reconciliation in `resolve.rs` and `version_format.rs`; external tag formats (`n8n@x`, `v` prefixes) remain. | +| SPEC-M06 | The workaround for variants that re-declare `upstream` (`package.rs`). | +| SPEC-T01 | Phantom root packages in dashboard manifest merging (`source/manifests.rs`) and root-only pin cleanup. | +| SPEC-T02 | The `variants/` vs `package_variants/` dual layout (`bump/package/types.rs`, `discovery.rs`). | +| SPEC-M02, SPEC-T03 | Divergent name and variant regexes across modules collapse into one. | + +Issues in tropibot independent of the spec: + +- Pin cleanup on branch close or delete uses only the root manifest `name`, so it errors for the 22 templates whose root has no name and misses variant packages. The SDK now cleans root and variant names (`cleanPinsFromDeletedBranches`); tropibot should do the same or leave cleanup to the SDK. +- Pinata `pinList` is not paginated (default page size 10), so cleanup sees only the first page. +- tropibot unpins when a PR closes even if its branch still exists; the SDK only cleans deleted branches. +- `docs/PACKAGE_CREATION_GUIDE.md` recommends a `variants//` layout with per-variant Dockerfiles, which contradicts SPEC-T02 and every real template. +- Two upstream bumpers exist: tropibot (46 repositories via workflows) and the SDK's `github-action bump-upstream` (5 repositories). One should become canonical. + +Contract between the SDK and tropibot that the spec work must keep: + +- The build comment markers `` and ``. tropibot parses the JSON with `deny_unknown_fields`, so spec findings must not be added to it; they are reported through logs and annotations instead. +- Git tags `v` and `@v` joined by `_`; release assets `_content-hash` and `_dappnode_package.json`. +- Pin metadata keys `dnpName`, `version`, `upstreamVersion`, `commit` and `branch`. +- `package_variants//releases.json`, read by the staking workflows with `jq 'to_entries | last'`, so its key order matters. + +## Checking a package locally + +`dappnodesdk build --skip_save` runs the checks without saving images. To see what strict mode would do: + +```sh +DAPPNODE_SDK_SPEC=error dappnodesdk build --skip_save +``` diff --git a/src/commands/build/handler.ts b/src/commands/build/handler.ts index f67b4b7c..c6f66e47 100644 --- a/src/commands/build/handler.ts +++ b/src/commands/build/handler.ts @@ -1,4 +1,6 @@ import { getRendererOptions } from "../../utils/progress.js"; +import { SpecFinding } from "../../spec/rules.js"; +import { printSpecReport } from "../../spec/report.js"; import Listr from "listr"; import { Architecture, architectures } from "@dappnode/types"; import { buildAndUpload } from "../../tasks/buildAndUpload/index.js"; @@ -60,6 +62,7 @@ export async function buildHandler({ } } + const specFindings: SpecFinding[] = []; const buildOptions: BuildAndUploadOptions = { dir, contentProvider, @@ -73,7 +76,8 @@ export async function buildHandler({ variantsDirPath, skipComposeValidation, stripComposeVersion, - packagesToBuildProps + packagesToBuildProps, + specFindings }; const verbosityOptions: VerbosityOptions = getRendererOptions({ @@ -83,7 +87,11 @@ export async function buildHandler({ const buildTasks = new Listr(buildAndUpload(buildOptions), verbosityOptions); - return await buildTasks.run(); + try { + return await buildTasks.run(); + } finally { + printSpecReport(specFindings); + } } const archShorthands: Record = { diff --git a/src/commands/publish/handler.ts b/src/commands/publish/handler.ts index 81c6c3c7..a2af01e6 100644 --- a/src/commands/publish/handler.ts +++ b/src/commands/publish/handler.ts @@ -1,4 +1,6 @@ import { getRendererOptions } from "../../utils/progress.js"; +import { SpecFinding } from "../../spec/rules.js"; +import { printSpecReport } from "../../spec/report.js"; import Listr from "listr"; import path from "path"; import { @@ -69,6 +71,7 @@ export async function publishHandler({ const isMultiVariant = !!allVariants || !!variants; + const specFindings: SpecFinding[] = []; const publishTasks = new Listr( publish({ releaseType, @@ -86,6 +89,7 @@ export async function publishHandler({ variantsDirPath, skipComposeValidation, stripComposeVersion, + specFindings, packagesToBuildProps: getPackagesToBuildProps({ allVariants: Boolean(allVariants), commaSeparatedVariants: variants, @@ -98,5 +102,9 @@ export async function publishHandler({ verbosityOptions ); - return await publishTasks.run(); + try { + return await publishTasks.run(); + } finally { + printSpecReport(specFindings); + } } diff --git a/src/spec/checkPackageSpec.ts b/src/spec/checkPackageSpec.ts new file mode 100644 index 00000000..e4faeb96 --- /dev/null +++ b/src/spec/checkPackageSpec.ts @@ -0,0 +1,679 @@ +import fs from "fs"; +import path from "path"; +import yaml from "js-yaml"; +import { createRequire } from "module"; +import { Compose, Manifest, releaseFiles } from "@dappnode/types"; +import { PackageToBuildProps } from "../types.js"; +import { defaultComposeFileName, defaultVariantsDirName } from "../params.js"; +import { parseGithubRepoSlug } from "../files/manifest/getRepoSlugFromManifest.js"; +import { shell } from "../utils/shell.js"; +import { SpecFinding, SpecRuleId } from "./rules.js"; + +const manifestFileName = "dappnode_package.json"; +const packageNamePattern = + /^[a-z0-9]+(-[a-z0-9]+)*\.(dnp|public)\.dappnode\.eth$/; +const variantNamePattern = /^[a-z0-9]+(-[a-z0-9]+)*$/; +/** Supported by DAPPMANAGER but not yet in the published manifest schema. */ +const knownUnreleasedManifestFields = ["upnpDisable"]; +/** Single release files that are copied (not merged) from the root or the variant. */ +const singleCopiedReleaseFiles = [ + "avatar", + "prometheusTargets", + "disclaimer", + "gettingStarted", + "setupSchema", + "setupTarget", + "setupUiJson", + "signature" +] as const; +/** Names that look like a release file of the given type; the type's regex decides if it is used. */ +const lookalikes: { + type: keyof typeof releaseFiles; + pattern: RegExp; + expected: string; +}[] = [ + { + type: "notifications", + pattern: /notifications\.[a-z]+$/i, + expected: "notifications.yaml" + }, + { + type: "gettingStarted", + pattern: /getting[-_ ]?started(\.[a-z]+)?$/i, + expected: "getting-started.md" + }, + { + type: "disclaimer", + pattern: /disclaimer(\.[a-z]+)?$/i, + expected: "disclaimer.md" + }, + { + type: "avatar", + pattern: /avatar.*\.(jpe?g|svg|webp|gif|ico)$/i, + expected: "avatar.png" + }, + { + type: "setupWizard", + pattern: /setup[-_]?wizard.*\.[a-z]+$/i, + expected: "setup-wizard.json or setup-wizard.yml" + }, + { + type: "prometheusTargets", + pattern: /prometheus[-_]?targets?.*\.[a-z]+$/i, + expected: "prometheus-targets.json" + }, + { + type: "grafanaDashboards", + pattern: /grafana[-_]?dashboard.*\.[a-z]+$/i, + expected: "-grafana-dashboard.json" + } +]; + +export interface SpecCheckInput { + rootDir: string; + variantsDirPath?: string; + composeFileName?: string; + packagesToBuildProps: PackageToBuildProps[]; + skipComposeValidation?: boolean; + /** GitHub "owner/repo" of the package repository, if known. */ + repoSlug?: string | null; + env?: NodeJS.ProcessEnv; +} + +/** + * Checks a package against the stricter package spec and returns every violation. + * It never throws for a spec violation; unreadable files are reported by validation. + */ +export function checkPackageSpec({ + rootDir, + variantsDirPath = path.join(rootDir, defaultVariantsDirName), + composeFileName = defaultComposeFileName, + packagesToBuildProps, + skipComposeValidation, + repoSlug, + env = process.env +}: SpecCheckInput): SpecFinding[] { + const findings: SpecFinding[] = []; + const add = (rule: SpecRuleId, message: string, file?: string) => + findings.push({ rule, message, file }); + + const variantDirs = listVariantDirs(variantsDirPath); + const isTemplate = variantDirs.length > 0; + const sourceDirs = [rootDir, ...variantDirs.map(v => v.dir)]; + const rootManifest = readRawManifest(rootDir); + + // --- Build inputs + if (skipComposeValidation) + add( + "SPEC-B01", + "Compose validation was skipped with --skip_compose_validation; DAPPMANAGER validates the compose again at install." + ); + if (env.UPSTREAM_VERSION) + add( + "SPEC-B02", + "The UPSTREAM_VERSION environment variable is set; declare the upstream version in the manifest and compose instead." + ); + if (composeFileName !== defaultComposeFileName) + add( + "SPEC-C01", + `The compose file is "${composeFileName}"; releases need ${defaultComposeFileName} (DAPPMANAGER only recognises compose*.yml).` + ); + + // --- Per source directory (root and each variant) + for (const dir of sourceDirs) { + const files = listFiles(dir); + const manifests = files.filter(file => + releaseFiles.manifest.regex.test(file) + ); + if (manifests.length > 1) + add( + "SPEC-M01", + `Several manifest files: ${manifests.join( + ", " + )}. Keep only ${manifestFileName}.`, + path.join(dir, manifests[0]) + ); + else if (manifests.length === 1 && manifests[0] !== manifestFileName) + add( + "SPEC-M01", + `Manifest is "${manifests[0]}"; rename it to ${manifestFileName}.`, + path.join(dir, manifests[0]) + ); + + const isReleaseFile = (file: string) => + Object.values(releaseFiles).some(spec => spec.regex.test(file)); + for (const file of files.filter(file => !isReleaseFile(file))) + for (const { type, pattern, expected } of lookalikes) + if (pattern.test(file)) + add( + "SPEC-F01", + `"${file}" looks like the ${type} release file but does not match its name pattern, so it is left out of the release; rename it to ${expected}.`, + path.join(dir, file) + ); + + for (const [type, spec] of Object.entries(releaseFiles)) { + if (type === "manifest" || type === "compose") continue; + for (const file of files.filter(file => spec.regex.test(file))) { + const size = fs.statSync(path.join(dir, file)).size; + if (size > spec.maxSize) + add( + "SPEC-F02", + `"${file}" is ${formatSize( + size + )}; the ${type} limit is ${formatSize(spec.maxSize)}.`, + path.join(dir, file) + ); + } + } + + const raw = readRawManifest(dir); + if (raw) { + const legacy = ["upstreamRepo", "upstreamVersion", "upstreamArg"].filter( + key => key in raw.manifest + ); + if (legacy.length > 0) + add( + "SPEC-M03", + `Uses the legacy ${legacy.join( + "/" + )} field(s); declare "upstream": [{ "repo", "version", "arg" }] instead.`, + raw.file + ); + const known = getManifestSchemaFields(); + const unknown = Object.keys(raw.manifest).filter(key => !known.has(key)); + if (unknown.length > 0) + add( + "SPEC-M07", + `Fields not in the manifest schema: ${unknown.join( + ", " + )}. DAPPMANAGER ignores or strips them.`, + raw.file + ); + } + + const composePath = path.join(dir, composeFileName); + if (fs.existsSync(composePath)) { + const text = fs.readFileSync(composePath, "utf8"); + // Compose consumes dollar signs in escaped pairs before interpolating. + const interpolated = [ + ...new Set( + [...text.matchAll(/(\$+)\{[^}]*\}/g)] + .filter(match => match[1].length % 2 === 1) + .map(match => match[0].slice(match[1].length - 1)) + ) + ]; + if (interpolated.length > 0) + add( + "SPEC-C04", + `Uses environment interpolation ${interpolated + .slice(0, 3) + .join(", ")}${ + interpolated.length > 3 ? ", ..." : "" + }; write the values out.`, + composePath + ); + const compose = loadYamlObject(text) as Partial | null; + for (const [service, def] of Object.entries(compose?.services || {})) + if ( + def && + typeof def.build === "object" && + Array.isArray(def.build.args) + ) + add( + "SPEC-C05", + `Service "${service}" lists build.args as "KEY=value" items; use a mapping (KEY: value).`, + composePath + ); + } + } + + // --- Templates + if (isTemplate) { + if ( + rootManifest && + (rootManifest.manifest.name || rootManifest.manifest.version) + ) + add( + "SPEC-T01", + `The root manifest sets ${["name", "version"] + .filter(key => key in rootManifest.manifest) + .join( + " and " + )} although the package has variants; set them in each variant.`, + rootManifest.file + ); + if (path.basename(variantsDirPath) !== defaultVariantsDirName) + add( + "SPEC-T02", + `Variants are in "${path.basename( + variantsDirPath + )}/"; move them to ${defaultVariantsDirName}/.`, + variantsDirPath + ); + for (const variant of variantDirs) { + if (!variantNamePattern.test(variant.name)) + add( + "SPEC-T03", + `Variant "${variant.name}" must use lowercase letters, digits and single hyphens.`, + variant.dir + ); + const raw = readRawManifest(variant.dir); + if (!raw) { + add( + "SPEC-T05", + `Variant "${variant.name}" has no ${manifestFileName}.`, + variant.dir + ); + continue; + } + const rootUpstream = rootManifest?.manifest.upstream; + const variantUpstream = raw.manifest.upstream; + if (Array.isArray(rootUpstream) && Array.isArray(variantUpstream)) { + // A shorter or longer list is fine: the merge keeps or appends the remaining entries. + const mismatch = variantUpstream.some( + (item, i) => rootUpstream[i] && item?.repo !== rootUpstream[i]?.repo + ); + if (mismatch) + add( + "SPEC-M06", + `Variant upstream[] (${variantUpstream + .map(item => item?.repo) + .join( + ", " + )}) puts a different repo at a position of the root's upstream[] (${rootUpstream + .map(item => item?.repo) + .join(", ")}); entries are merged by position.`, + raw.file + ); + } + for (const type of singleCopiedReleaseFiles) { + const inRoot = listFiles(rootDir).find(file => + releaseFiles[type].regex.test(file) + ); + const inVariant = listFiles(variant.dir).find(file => + releaseFiles[type].regex.test(file) + ); + if (inRoot && inVariant) + add( + "SPEC-T04", + `${type} exists in the root (${inRoot}) and in variant "${variant.name}" (${inVariant}); the variant copy replaces the root copy. Keep only one.`, + path.join(variant.dir, inVariant) + ); + } + } + } + + // --- Per built package (merged root + variant). Each finding points at the file that + // defines the value, so a value inherited from the root is reported once, not per variant. + for (const pkg of packagesToBuildProps) { + const { manifest, compose } = pkg; + const manifestDirs = pkg.manifestPaths.map(spec => spec.dir || rootDir); + const composeDirs = pkg.composePaths.map(spec => spec.dir || rootDir); + const where = (dir: string) => + dir === rootDir ? "" : ` (variant "${pkg.variant}")`; + const manifestOrigin = (has: (m: Partial) => boolean) => + definingDir(manifestDirs, dir => readRawManifest(dir)?.manifest, has); + const composeOrigin = (has: (c: Partial) => boolean) => + definingDir( + composeDirs, + dir => readRawCompose(dir, composeFileName), + has + ); + const manifestFileIn = (dir: string) => + readRawManifest(dir)?.file || path.join(dir, manifestFileName); + const composeFileIn = (dir: string) => path.join(dir, composeFileName); + + { + const dir = manifestOrigin(m => "name" in m); + if (!packageNamePattern.test(manifest.name || "")) + add( + "SPEC-M02", + `Package name "${manifest.name}"${where( + dir + )} must match .dnp.dappnode.eth or .public.dappnode.eth.`, + manifestFileIn(dir) + ); + } + + // Fresh merge of the source files: the build mutates upstreamVersion on its copy. + const sourceManifest = mergedSourceManifest(pkg) || manifest; + const buildArgs = collectBuildArgs(compose); + const usesUpstreamArray = Array.isArray(sourceManifest.upstream); + const upstreams = usesUpstreamArray + ? sourceManifest.upstream || [] + : sourceManifest.upstreamRepo + ? [ + { + repo: sourceManifest.upstreamRepo, + version: sourceManifest.upstreamVersion, + arg: sourceManifest.upstreamArg + } + ] + : []; + const upstreamDir = manifestOrigin(m => + usesUpstreamArray + ? "upstream" in m + : "upstreamRepo" in m || "upstreamArg" in m || "upstreamVersion" in m + ); + for (const item of upstreams) { + if (!item?.repo) continue; + // The legacy fields default the arg to UPSTREAM_VERSION; check the version against it too. + const upstream = { ...item, arg: item.arg || "UPSTREAM_VERSION" }; + if (!item.arg) { + add( + "SPEC-M04", + buildArgs.has(upstream.arg) + ? `Upstream ${upstream.repo}${where( + upstreamDir + )} relies on the implicit build arg UPSTREAM_VERSION; name it explicitly as the upstream arg.` + : `Upstream ${upstream.repo}${where( + upstreamDir + )} has no arg, and no compose service declares the default build arg UPSTREAM_VERSION.`, + manifestFileIn(upstreamDir) + ); + if (!buildArgs.has(upstream.arg)) continue; + } + const values = buildArgs.get(upstream.arg); + if (!values) { + add( + "SPEC-M04", + `Upstream ${upstream.repo}${where(upstreamDir)} names arg "${ + upstream.arg + }", which no compose service declares in build.args.`, + manifestFileIn(upstreamDir) + ); + continue; + } + // "@sha256:" pins the same version by digest. + const matchesVersion = (value: string) => + value === String(upstream.version) || + value.startsWith(`${upstream.version}@sha256:`); + if (upstream.version !== undefined && ![...values].some(matchesVersion)) { + const argDir = composeOrigin(c => + Object.values(c.services || {}).some( + def => + isObject(def?.build) && + hasBuildArg(def.build, upstream.arg as string) + ) + ); + const variant = where(upstreamDir) || where(argDir); + add( + "SPEC-M05", + `Upstream ${upstream.repo}${variant} has version "${ + upstream.version + }" but build arg ${upstream.arg} is "${[...values].join('", "')}".`, + manifestFileIn(upstreamDir) + ); + } + } + + const services = Object.keys(compose.services || {}); + for (const exposable of manifest.exposable || []) + if (exposable.serviceName && !services.includes(exposable.serviceName)) { + const dir = manifestOrigin(m => "exposable" in m); + add( + "SPEC-M09", + `Exposable "${exposable.name}"${where(dir)} refers to service "${ + exposable.serviceName + }", which is not in the compose.`, + manifestFileIn(dir) + ); + } + + for (const [service, def] of Object.entries(compose.services || {})) { + // Core packages keep container_name: the installer starts and addresses them by name. + if (def.container_name && manifest.type !== "dncore") { + const dir = composeOrigin(c => + Boolean(c.services?.[service]?.container_name) + ); + add( + "SPEC-C02", + `Service "${service}"${where( + dir + )} sets container_name, which DAPPMANAGER replaces at install; remove it.`, + composeFileIn(dir) + ); + } + if (!def.build && def.image && !isPinnedImage(def.image)) { + const dir = composeOrigin(c => Boolean(c.services?.[service]?.image)); + add( + "SPEC-C03", + `External image "${def.image}" of service "${service}"${where( + dir + )} is not pinned; use an explicit version tag or digest.`, + composeFileIn(dir) + ); + } + } + + if (repoSlug && manifest.repository?.url) { + const declared = parseGithubRepoSlug(manifest.repository.url); + if (declared.toLowerCase() !== repoSlug.toLowerCase()) { + const dir = manifestOrigin(m => "repository" in m); + add( + "SPEC-M08", + `repository.url${where(dir)} is "${ + manifest.repository.url + }" but the package repository is ${repoSlug}.`, + manifestFileIn(dir) + ); + } + } + + const label = pkg.variant ? ` (variant "${pkg.variant}")` : ""; + const composeSize = Buffer.byteLength(yaml.dump(compose)); + if (composeSize > releaseFiles.compose.maxSize) + add( + "SPEC-F02", + `The release compose${label} is about ${formatSize( + composeSize + )}; the limit is ${formatSize(releaseFiles.compose.maxSize)}.`, + composeFileIn(composeDirs[composeDirs.length - 1]) + ); + const manifestSize = Buffer.byteLength(JSON.stringify(manifest, null, 2)); + if (manifestSize > releaseFiles.manifest.maxSize) + add( + "SPEC-F02", + `The release manifest${label} is about ${formatSize( + manifestSize + )}; the limit is ${formatSize(releaseFiles.manifest.maxSize)}.`, + manifestFileIn(manifestDirs[manifestDirs.length - 1]) + ); + } + + return dedupe(findings); +} + +/** GitHub "owner/repo" of the package: GITHUB_REPOSITORY in Actions, otherwise the origin remote. */ +export async function getPackageRepoSlug( + rootDir: string, + env: NodeJS.ProcessEnv = process.env +): Promise { + if (env.GITHUB_REPOSITORY) return env.GITHUB_REPOSITORY; + try { + const remote = ( + await shell(["git", "remote", "get-url", "origin"], { cwd: rootDir }) + ).trim(); + const ssh = remote.match(/^git@github\.com:([^/]+\/[^/]+?)(\.git)?$/); + return ssh ? ssh[1] : parseGithubRepoSlug(remote) || null; + } catch { + return null; + } +} + +function listVariantDirs( + variantsDirPath: string +): { name: string; dir: string }[] { + if (!fs.existsSync(variantsDirPath)) return []; + return fs + .readdirSync(variantsDirPath, { withFileTypes: true }) + .filter(entry => entry.isDirectory()) + .map(entry => ({ + name: entry.name, + dir: path.join(variantsDirPath, entry.name) + })); +} + +function listFiles(dir: string): string[] { + try { + return fs + .readdirSync(dir, { withFileTypes: true }) + .filter(entry => entry.isFile()) + .map(entry => entry.name); + } catch { + return []; + } +} + +function readRawManifest( + dir: string +): { file: string; manifest: Partial } | null { + const file = listFiles(dir).find(name => + releaseFiles.manifest.regex.test(name) + ); + if (!file) return null; + const manifest = loadYamlObject( + fs.readFileSync(path.join(dir, file), "utf8") + ); + return manifest + ? { file: path.join(dir, file), manifest: manifest as Partial } + : null; +} + +/** The last directory (root first, variant last) whose file defines the value, else the root. */ +function definingDir( + dirs: string[], + read: (dir: string) => T | null | undefined, + has: (value: T) => boolean +): string { + for (const dir of [...dirs].reverse()) { + const value = read(dir); + if (value && has(value)) return dir; + } + return dirs[0]; +} + +function readRawCompose( + dir: string, + composeFileName: string +): Partial | null { + const file = path.join(dir, composeFileName); + if (!fs.existsSync(file)) return null; + return loadYamlObject( + fs.readFileSync(file, "utf8") + ) as Partial | null; +} + +function hasBuildArg(build: unknown, arg: string): boolean { + const args = isObject(build) ? build.args : undefined; + if (Array.isArray(args)) + return args.some(item => String(item).split("=")[0] === arg); + return isObject(args) && arg in args; +} + +/** Root and variant manifests merged the way readManifest merges them, before the build mutates them. */ +function mergedSourceManifest( + pkg: PackageToBuildProps +): Partial | null { + const parts = pkg.manifestPaths.map( + spec => readRawManifest(spec.dir || ".")?.manifest + ); + if (parts.some(part => !part)) return null; + return mergeByPosition(parts as Partial[]); +} + +function mergeByPosition(parts: T[]): T { + const merge = (base: unknown, override: unknown): unknown => { + if (Array.isArray(base) && Array.isArray(override)) { + const out = [...base]; + override.forEach((item, i) => (out[i] = merge(base[i], item))); + return out; + } + if (isObject(base) && isObject(override)) { + const out: Record = { ...base }; + for (const [key, value] of Object.entries(override)) + out[key] = merge(base[key], value); + return out; + } + return override === undefined ? base : override; + }; + return parts.reduce((acc, part) => merge(acc, part) as T, {} as T); +} + +/** Build arg name -> values across all services; list-style "KEY=value" args included. */ +function collectBuildArgs(compose: Compose): Map> { + const args = new Map>(); + const put = (key: string, value: unknown) => { + if (!args.has(key)) args.set(key, new Set()); + args + .get(key) + ?.add(value === undefined || value === null ? "" : String(value)); + }; + for (const def of Object.values(compose.services || {})) { + if (!def || typeof def.build !== "object" || !def.build.args) continue; + const buildArgs: unknown = def.build.args; + if (Array.isArray(buildArgs)) + for (const item of buildArgs) { + const [key, ...rest] = String(item).split("="); + put(key, rest.join("=")); + } + else if (isObject(buildArgs)) + for (const [key, value] of Object.entries(buildArgs)) put(key, value); + } + return args; +} + +function isPinnedImage(image: string): boolean { + if (image.includes("@sha256:")) return true; + const lastSegment = image.split("/").pop() || ""; + const tag = lastSegment.includes(":") + ? lastSegment.split(":").pop() + : undefined; + return Boolean(tag) && tag !== "latest"; +} + +let manifestSchemaFields: Set | undefined; +function getManifestSchemaFields(): Set { + if (!manifestSchemaFields) { + const require = createRequire(import.meta.url); + const schemaPath = path.join( + path.dirname(require.resolve("@dappnode/schemas/package.json")), + "dist/schemas/manifest.schema.json" + ); + const schema = JSON.parse(fs.readFileSync(schemaPath, "utf8")); + manifestSchemaFields = new Set([ + ...Object.keys(schema.properties || {}), + ...knownUnreleasedManifestFields + ]); + } + return manifestSchemaFields; +} + +function loadYamlObject(text: string): Record | null { + try { + const value = yaml.load(text); + return isObject(value) ? value : null; + } catch { + return null; + } +} + +function isObject(value: unknown): value is Record { + return typeof value === "object" && value !== null && !Array.isArray(value); +} + +function formatSize(bytes: number): string { + return bytes >= 1e6 + ? `${(bytes / 1e6).toFixed(1)} MB` + : `${Math.round(bytes / 1e3)} KB`; +} + +function dedupe(findings: SpecFinding[]): SpecFinding[] { + const seen = new Set(); + return findings.filter(finding => { + const key = `${finding.rule}|${finding.file || ""}|${finding.message}`; + if (seen.has(key)) return false; + seen.add(key); + return true; + }); +} diff --git a/src/spec/report.ts b/src/spec/report.ts new file mode 100644 index 00000000..d55200de --- /dev/null +++ b/src/spec/report.ts @@ -0,0 +1,98 @@ +import path from "path"; +import chalk from "chalk"; +import { SpecFinding, specDocUrl, specRules } from "./rules.js"; + +/** + * warn (default): report violations, never fail. + * error: fail validation on any violation (opt-in today, the planned default). + * off: skip the checks. + */ +export type SpecMode = "warn" | "error" | "off"; + +export function getSpecMode(env: NodeJS.ProcessEnv = process.env): SpecMode { + const value = (env.DAPPNODE_SDK_SPEC || "").trim().toLowerCase(); + return value === "error" || value === "off" ? value : "warn"; +} + +function relative(file: string | undefined, base: string): string { + if (!file) return ""; + const rel = path.relative(base, file); + return rel && !rel.startsWith("..") ? rel : file; +} + +export function formatSpecFindings( + findings: SpecFinding[], + { base = process.cwd() }: { base?: string } = {} +): string[] { + return findings.map(({ rule, message, file }) => { + const where = relative(file, base); + return `${rule}${where ? ` ${where}` : ""}: ${message}`; + }); +} + +export function formatSpecReport( + findings: SpecFinding[], + { + base = process.cwd(), + mode = "warn" + }: { base?: string; mode?: SpecMode } = {} +): string { + const count = `${findings.length} package spec ${ + findings.length === 1 ? "violation" : "violations" + }`; + const header = + mode === "error" + ? `${chalk.red("✖")} ${count}:` + : `${chalk.yellow( + "⚠" + )} ${count}. These will become errors in a future SDK release:`; + return [ + header, + ...formatSpecFindings(findings, { base }).map(line => ` ${line}`), + ` See ${specDocUrl}` + ].join("\n"); +} + +/** GitHub Actions workflow commands, shown as annotations on the run and the pull request. */ +export function formatGithubAnnotations( + findings: SpecFinding[], + { + base = process.cwd(), + level = "warning" + }: { base?: string; level?: "warning" | "error" } = {} +): string[] { + const escapeData = (value: string) => + value.replace(/%/g, "%25").replace(/\r/g, "%0D").replace(/\n/g, "%0A"); + const escapeProperty = (value: string) => + escapeData(value).replace(/:/g, "%3A").replace(/,/g, "%2C"); + return findings.map(({ rule, message, file }) => { + const props = [ + ...(file ? [`file=${escapeProperty(relative(file, base))}`] : []), + `title=${escapeProperty(`${rule} ${specRules[rule].title}`)}` + ]; + return `::${level} ${props.join(",")}::${escapeData(message)}`; + }); +} + +/** + * Prints the findings once, after the task renderer has finished. In error mode the + * failed validation already listed them, so only the GitHub annotations are added. + */ +export function printSpecReport( + findings: SpecFinding[], + { + env = process.env, + base = env.GITHUB_WORKSPACE || process.cwd() + }: { env?: NodeJS.ProcessEnv; base?: string } = {} +): void { + if (findings.length === 0) return; + const mode = getSpecMode(env); + if (mode !== "error") + console.error(`\n${formatSpecReport(findings, { base, mode })}\n`); + if (env.GITHUB_ACTIONS === "true") + for (const line of formatGithubAnnotations(findings, { + base, + level: mode === "error" ? "error" : "warning" + })) + console.log(line); +} diff --git a/src/spec/rules.ts b/src/spec/rules.ts new file mode 100644 index 00000000..e4a6660f --- /dev/null +++ b/src/spec/rules.ts @@ -0,0 +1,122 @@ +/** + * DAppNode package spec rules (doc/package-spec.md). + * + * The SDK reports violations as warnings for now. Each rule has a stable id so + * that workflows, tropibot and package maintainers can refer to it, and so the + * same rule can later be turned into an error without changing its meaning. + */ +export const specRules = { + // Manifest + "SPEC-M01": { + title: "One manifest named dappnode_package.json", + why: "sdk-publish, the ISO installer and tropibot read dappnode_package.json by name; with several candidates the SDK picks one arbitrarily." + }, + "SPEC-M02": { + title: "Package name format", + why: "Names must be .dnp.dappnode.eth or .public.dappnode.eth with lowercase letters, digits and single hyphens." + }, + "SPEC-M03": { + title: "Upstream declared with upstream[]", + why: "The legacy upstreamRepo/upstreamVersion/upstreamArg fields need separate code paths in the SDK, tropibot and the dashboards." + }, + "SPEC-M04": { + title: "Upstream arg is a compose build arg", + why: "Bumps edit the build arg named by `arg`; without it tools have to guess which arg holds the version." + }, + "SPEC-M05": { + title: "Upstream version equals the build arg value", + why: "Manifest and compose must describe the same upstream version; tools otherwise have to translate between tag formats." + }, + "SPEC-M06": { + title: "Variant upstream[] matches the root order", + why: "Root and variant manifests are merged by array position, so a reordered or shortened variant upstream[] mixes entries." + }, + "SPEC-M07": { + title: "Only known manifest fields", + why: "Fields outside the manifest schema are ignored or stripped by DAPPMANAGER." + }, + "SPEC-M08": { + title: "repository.url points to the package repository", + why: "GitHub releases and bots use the manifest repository; another repository's URL targets the wrong repo." + }, + "SPEC-M09": { + title: "Exposable services exist in compose", + why: "An exposable entry for a service that is not in the compose cannot be exposed." + }, + // Templates (multi-variant packages) + "SPEC-T01": { + title: "Template root manifest has no name or version", + why: "With variants, every package identity comes from its variant; a named root becomes an extra phantom package for cleanup and dashboards." + }, + "SPEC-T02": { + title: "Variants live in package_variants/", + why: "Workflows, tropibot and the shared staking tests only look in package_variants/." + }, + "SPEC-T03": { + title: "Variant directory names", + why: "Variant names appear in git tags (@v) and ENS names; use lowercase letters, digits and single hyphens." + }, + "SPEC-T04": { + title: "A single release file lives in the root or the variant, not both", + why: "The variant copy silently replaces the root copy, so the root copy is dead and easily goes stale." + }, + "SPEC-T05": { + title: "Every variant directory has a manifest", + why: "A variant without dappnode_package.json cannot be built or published." + }, + // Compose + "SPEC-C01": { + title: "Compose file named docker-compose.yml", + why: "DAPPMANAGER only recognises compose*.yml in a release, and the ISO installer and tropibot read docker-compose.yml." + }, + "SPEC-C02": { + title: "No container_name", + why: "DAPPMANAGER replaces container_name at install, so the value has no effect." + }, + "SPEC-C03": { + title: "External images are pinned", + why: "An untagged or :latest external image makes the release content depend on when it was built." + }, + "SPEC-C04": { + title: "No ${VAR} interpolation in compose", + why: "Interpolation depends on the environment of whoever runs compose; releases must be self-contained." + }, + "SPEC-C05": { + title: "build.args is a mapping", + why: "List-style args (- KEY=value) need separate parsing in every tool that edits upstream versions." + }, + // Release files + "SPEC-F01": { + title: "Release files use the recognised names", + why: "A file that looks like a release file but does not match its expected name is silently left out of the release." + }, + "SPEC-F02": { + title: "Release files stay within the size limits", + why: "DAPPMANAGER rejects release files above the limits in @dappnode/types releaseFiles." + }, + "SPEC-F03": { + title: "Values have the schema types", + why: 'Schema validation coerces values (e.g. isBanner: "false" becomes false); the source should already have the right type.' + }, + // Build inputs + "SPEC-B01": { + title: "Compose validation is not skipped", + why: "DAPPMANAGER validates the compose again at install, so a release built with --skip_compose_validation may not install." + }, + "SPEC-B02": { + title: "No UPSTREAM_VERSION environment override", + why: "The upstream version must come from the package files, not from the environment of the build." + } +} as const satisfies Record; + +export type SpecRuleId = keyof typeof specRules; + +export interface SpecFinding { + rule: SpecRuleId; + message: string; + /** Absolute path of the file the finding is about, if any. */ + file?: string; +} + +export const specDocUrl = + "https://github.com/dappnode/DAppNodeSDK/blob/master/doc/package-spec.md"; diff --git a/src/tasks/buildAndUpload/getFileValidationTask.ts b/src/tasks/buildAndUpload/getFileValidationTask.ts index 7672e14f..9ed6b79b 100644 --- a/src/tasks/buildAndUpload/getFileValidationTask.ts +++ b/src/tasks/buildAndUpload/getFileValidationTask.ts @@ -13,35 +13,83 @@ import { import { cloneDeep } from "lodash-es"; import { CliError } from "../../params.js"; import { shell, ShellError } from "../../utils/shell.js"; +import { + checkPackageSpec, + getPackageRepoSlug +} from "../../spec/checkPackageSpec.js"; +import { SpecFinding } from "../../spec/rules.js"; +import { formatSpecFindings, getSpecMode } from "../../spec/report.js"; export function getFileValidationTask({ packagesToBuildProps, skipComposeValidation, - stripComposeVersion + stripComposeVersion, + rootDir, + variantsDirPath, + composeFileName, + specFindings = [] }: { packagesToBuildProps: PackageToBuildProps[]; skipComposeValidation?: boolean; stripComposeVersion?: boolean; + rootDir?: string; + variantsDirPath?: string; + composeFileName?: string; + /** Collects package spec violations; the command prints them after the run. */ + specFindings?: SpecFinding[]; }): ListrTask { return { title: `Validate files`, task: async (ctx, task) => { + const mode = getSpecMode(); + // Spec checks read the source files, so they run before validation can mutate anything. + if (mode !== "off" && rootDir) { + onStatus("Checking package spec"); + specFindings.push( + ...checkPackageSpec({ + rootDir, + variantsDirPath, + composeFileName, + packagesToBuildProps, + skipComposeValidation, + repoSlug: await getPackageRepoSlug(rootDir) + }) + ); + } + const coercions = await validatePackageFiles({ packagesToBuildProps, skipComposeValidation, stripComposeVersion, - onStatus: message => { - task.output = message; - } + onStatus }); - // Release files are written from the validated objects, so coerced values are - // what ships. Keep the warning in the title so it stays visible after completion. - if (coercions.length > 0) - task.title = `Validate files (warning: schema validation changed ${ - coercions.length - } value(s), fix the source files: ${coercions.slice(0, 5).join("; ")}${ - coercions.length > 5 ? "; ..." : "" - })`; + // Release files are written from the validated objects, so coerced values are what ships. + if (mode !== "off") + specFindings.push( + ...coercions.map(coercion => ({ + rule: "SPEC-F03" as const, + message: `Schema validation changed ${coercion}; write the value with the schema type.` + })) + ); + + if (specFindings.length > 0) { + if (mode === "error") + throw new CliError( + [ + `${specFindings.length} package spec violation(s) (DAPPNODE_SDK_SPEC=error):`, + ...formatSpecFindings(specFindings, { base: rootDir }).map( + line => ` ${line}` + ) + ].join("\n") + ); + task.title = `Validate files (${specFindings.length} spec ${ + specFindings.length === 1 ? "warning" : "warnings" + }, listed at the end)`; + } + + function onStatus(message: string): void { + task.output = message; + } } }; } diff --git a/src/tasks/buildAndUpload/index.ts b/src/tasks/buildAndUpload/index.ts index 877f91b0..b4d1c27f 100644 --- a/src/tasks/buildAndUpload/index.ts +++ b/src/tasks/buildAndUpload/index.ts @@ -29,7 +29,8 @@ export function buildAndUpload({ variantsDirPath = defaultVariantsDirName, packagesToBuildProps, skipComposeValidation, - stripComposeVersion + stripComposeVersion, + specFindings }: BuildAndUploadOptions): ListrTask[] { const buildTimeout = parseTimeout(userTimeout); @@ -43,7 +44,11 @@ export function buildAndUpload({ getFileValidationTask({ packagesToBuildProps, skipComposeValidation, - stripComposeVersion + stripComposeVersion, + rootDir: dir, + variantsDirPath, + composeFileName, + specFindings }), getVerifyConnectionTask({ releaseUploader, skipUpload }), getReleaseDirCreationTask({ packagesToBuildProps }), diff --git a/src/tasks/buildAndUpload/types.ts b/src/tasks/buildAndUpload/types.ts index 6e02dfb0..7bd05e25 100644 --- a/src/tasks/buildAndUpload/types.ts +++ b/src/tasks/buildAndUpload/types.ts @@ -1,5 +1,6 @@ import { PackageToBuildProps } from "../../types.js"; import { UploadTo } from "../../releaseUploader/index.js"; +import { SpecFinding } from "../../spec/rules.js"; export interface BuildAndUploadOptions { contentProvider: string; @@ -15,4 +16,6 @@ export interface BuildAndUploadOptions { packagesToBuildProps: PackageToBuildProps[]; skipComposeValidation?: boolean; stripComposeVersion?: boolean; + /** Collects package spec violations found during validation. */ + specFindings?: SpecFinding[]; } diff --git a/src/tasks/publish/index.ts b/src/tasks/publish/index.ts index 9ae740de..c52da692 100644 --- a/src/tasks/publish/index.ts +++ b/src/tasks/publish/index.ts @@ -25,7 +25,8 @@ export function publish({ packagesToBuildProps, isMultiVariant, skipComposeValidation, - stripComposeVersion + stripComposeVersion, + specFindings }: PublishOptions): ListrTask[] { return [ getVerifyEthConnectionTask({ ethProvider }), @@ -52,7 +53,8 @@ export function publish({ packagesToBuildProps, variantsDirPath, skipComposeValidation, - stripComposeVersion + stripComposeVersion, + specFindings }, verbosityOptions }), diff --git a/src/tasks/publish/types.ts b/src/tasks/publish/types.ts index baadc520..4236cbc7 100644 --- a/src/tasks/publish/types.ts +++ b/src/tasks/publish/types.ts @@ -1,3 +1,4 @@ +import { SpecFinding } from "../../spec/rules.js"; import { VerbosityOptions } from "../../commands/build/types.js"; import { UploadTo } from "../../releaseUploader/index.js"; import { PackageToBuildProps, ReleaseType } from "../../types.js"; @@ -20,4 +21,5 @@ export interface PublishOptions { isMultiVariant: boolean; skipComposeValidation?: boolean; stripComposeVersion?: boolean; + specFindings?: SpecFinding[]; } diff --git a/test/spec/checkPackageSpec.test.ts b/test/spec/checkPackageSpec.test.ts new file mode 100644 index 00000000..02f65953 --- /dev/null +++ b/test/spec/checkPackageSpec.test.ts @@ -0,0 +1,389 @@ +import fs from "fs"; +import path from "path"; +import yaml from "js-yaml"; +import Listr from "listr"; +import { expect } from "chai"; +import { initHandler } from "../../src/commands/init/handler.js"; +import { getPackagesToBuildProps } from "../../src/commands/build/variants.js"; +import { checkPackageSpec } from "../../src/spec/checkPackageSpec.js"; +import { + formatGithubAnnotations, + formatSpecReport, + getSpecMode +} from "../../src/spec/report.js"; +import { SpecFinding, specRules } from "../../src/spec/rules.js"; +import { getFileValidationTask } from "../../src/tasks/buildAndUpload/getFileValidationTask.js"; +import { temporaryDirectory } from "../helpers/fixtures.js"; + +function check( + dir: string, + extra: Partial[0]> = {} +): SpecFinding[] { + const variantsDirPath = path.join(dir, "package_variants"); + return checkPackageSpec({ + rootDir: dir, + variantsDirPath, + packagesToBuildProps: getPackagesToBuildProps({ + allVariants: fs.existsSync(variantsDirPath), + rootDir: dir, + variantsDirPath, + composeFileName: "docker-compose.yml" + }), + env: {}, + ...extra + }); +} + +const rulesOf = (findings: SpecFinding[]) => + [...new Set(findings.map(finding => finding.rule))].sort(); + +describe("package spec checks", function () { + this.timeout(30000); + let dir: string; + const write = (rel: string, content: string | object) => { + fs.mkdirSync(path.dirname(path.join(dir, rel)), { recursive: true }); + fs.writeFileSync( + path.join(dir, rel), + typeof content === "string" ? content : JSON.stringify(content, null, 2) + ); + }; + beforeEach(() => (dir = temporaryDirectory())); + afterEach(() => fs.rmSync(dir, { recursive: true, force: true })); + + it("finds nothing in packages created by init", async () => { + for (const useVariants of [false, true]) { + const pkgDir = path.join(dir, useVariants ? "template" : "single"); + fs.mkdirSync(pkgDir); + await initHandler({ + dir: pkgDir, + yes: true, + force: true, + use_variants: useVariants + }); + expect(check(pkgDir), useVariants ? "template" : "single").to.deep.equal( + [] + ); + } + }); + + it("reports each non-spec pattern with its rule and file", () => { + // Template root that also names a package (juno-generic), legacy upstream fields + // without an arg, and assorted compose and file-name issues. + write("dappnode_package.json", { + name: "demo.dnp.dappnode.eth", + version: "0.1.0", + upstreamRepo: "acme/demo", + upstreamVersion: "v1.0.0", + image: { path: "legacy" }, + repository: { type: "git", url: "https://github.com/acme/demo.git" } + }); + write( + "docker-compose.yml", + yaml.dump({ + version: "3.5", + services: { + app: { + build: { context: ".", args: ["UPSTREAM_VERSION=1.0.0"] }, + image: "app.demo.dnp.dappnode.eth:0.1.0", + container_name: "DAppNodePackage-demo", + environment: { PORT: "${PORT:-80}" } + }, + db: { image: "postgres" } + } + }) + ); + write("avatar-default.png", "png"); + write("notifications.yml", "endpoints: []\n"); + write("prometheus-targets.json", "[]"); + write("package_variants/mainnet/dappnode_package.json", { + name: "demo.dnp.dappnode.eth", + version: "0.1.0" + }); + write( + "package_variants/mainnet/docker-compose.yml", + yaml.dump({ version: "3.5", services: { app: {} } }) + ); + write("package_variants/mainnet/prometheus-targets.json", "[]"); + write("package_variants/Test_Net/dappnode_package.json", { + name: "demo-test.dnp.dappnode.eth", + version: "0.1.0" + }); + write( + "package_variants/Test_Net/docker-compose.yml", + yaml.dump({ version: "3.5", services: { app: {} } }) + ); + + const findings = check(dir, { + repoSlug: "dappnode/DAppNodePackage-demo", + skipComposeValidation: true, + env: { UPSTREAM_VERSION: "1.0.0" } + }); + expect(rulesOf(findings)).to.deep.equal([ + "SPEC-B01", + "SPEC-B02", + "SPEC-C02", + "SPEC-C03", + "SPEC-C04", + "SPEC-C05", + "SPEC-F01", + "SPEC-M03", + "SPEC-M04", + "SPEC-M05", + "SPEC-M07", + "SPEC-M08", + "SPEC-T01", + "SPEC-T03", + "SPEC-T04" + ]); + // A value inherited from the root is reported once, not once per variant. + expect( + findings.filter(finding => finding.rule === "SPEC-C02") + ).to.have.length(1); + expect( + findings.find(finding => finding.rule === "SPEC-C02")?.file + ).to.equal(path.join(dir, "docker-compose.yml")); + // The legacy fields rely on the implicit arg UPSTREAM_VERSION, whose value differs from upstreamVersion. + expect( + findings.find(finding => finding.rule === "SPEC-M04")?.message + ).to.include("implicit build arg UPSTREAM_VERSION"); + expect( + findings.find(finding => finding.rule === "SPEC-M05")?.message + ).to.include('version "v1.0.0"'); + }); + + it("accepts escaped compose variables without reporting interpolation", () => { + write("dappnode_package.json", { + name: "demo.dnp.dappnode.eth", + version: "0.1.0" + }); + write( + "docker-compose.yml", + yaml.dump({ + services: { + app: { + image: "alpine:3.21", + environment: { + HEALTH_PORT: "$${PORT:-9090}", + HEALTH_TOKEN: "$${ANON_KEY}", + LITERAL: "$$$${LITERAL}" + } + } + } + }) + ); + expect(check(dir)).to.deep.equal([]); + }); + + it("reports real interpolation after escaped dollar pairs", () => { + write("dappnode_package.json", { + name: "demo.dnp.dappnode.eth", + version: "0.1.0" + }); + write( + "docker-compose.yml", + yaml.dump({ + services: { + app: { + image: "alpine:3.21", + environment: { + PORT: "${PORT:-80}", + PREFIXED: "$$${TOKEN}", + REPEATED: "${PORT:-80}", + ESCAPED: "$${SAFE}" + } + } + } + }) + ); + const findings = check(dir).filter(finding => finding.rule === "SPEC-C04"); + expect(findings).to.have.length(1); + expect(findings[0].message).to.equal( + "Uses environment interpolation ${PORT:-80}, ${TOKEN}; write the values out." + ); + }); + + it("accepts digest-pinned args, shorter variant upstream lists and real avatars", () => { + write("dappnode_package.json", { + upstream: [ + { repo: "acme/app", version: "1.0.0", arg: "UPSTREAM_VERSION" }, + { + repo: "dappnode/staker-package-scripts", + version: "v0.1.2", + arg: "STAKER_SCRIPTS_VERSION" + } + ] + }); + write( + "docker-compose.yml", + yaml.dump({ + version: "3.5", + services: { + app: { + build: { + context: ".", + args: { + UPSTREAM_VERSION: "1.0.0@sha256:abc", + STAKER_SCRIPTS_VERSION: "v0.1.2" + } + }, + image: "app.demo.dnp.dappnode.eth:0.1.0" + } + } + }) + ); + write("avatar-notifications.png", "png"); + write("package_variants/mainnet/dappnode_package.json", { + name: "demo.dnp.dappnode.eth", + version: "0.1.0" + }); + write( + "package_variants/mainnet/docker-compose.yml", + yaml.dump({ version: "3.5", services: { app: {} } }) + ); + write( + "package_variants/lukso/docker-compose.yml", + yaml.dump({ version: "3.5", services: { app: {} } }) + ); + write("package_variants/lukso/dappnode_package.json", { + name: "demo-lukso.dnp.dappnode.eth", + version: "0.1.0", + upstream: [ + { repo: "acme/app", version: "1.0.0", arg: "UPSTREAM_VERSION" } + ] + }); + expect(check(dir)).to.deep.equal([]); + }); + + it("flags a variant that puts a different repo at a root upstream position", () => { + write("dappnode_package.json", { + upstream: [ + { repo: "acme/app", version: "1.0.0", arg: "UPSTREAM_VERSION" }, + { repo: "acme/tool", version: "2.0.0", arg: "TOOL_VERSION" } + ] + }); + write( + "docker-compose.yml", + yaml.dump({ + version: "3.5", + services: { + app: { + build: { + context: ".", + args: { UPSTREAM_VERSION: "1.0.0", TOOL_VERSION: "2.0.0" } + } + } + } + }) + ); + write( + "package_variants/main/docker-compose.yml", + yaml.dump({ version: "3.5", services: { app: {} } }) + ); + write("package_variants/main/dappnode_package.json", { + name: "demo.dnp.dappnode.eth", + version: "0.1.0", + upstream: [{ repo: "acme/tool", version: "2.0.0", arg: "TOOL_VERSION" }] + }); + expect(rulesOf(check(dir))).to.deep.equal(["SPEC-M06"]); + }); + + it("lets core packages keep container_name, which the installer relies on", () => { + write("dappnode_package.json", { + name: "bind.dnp.dappnode.eth", + version: "0.1.0", + type: "dncore" + }); + write( + "docker-compose.yml", + yaml.dump({ + version: "3.5", + services: { + "bind.dnp.dappnode.eth": { + build: ".", + image: "bind.dnp.dappnode.eth:0.1.0", + container_name: "DAppNodeCore-bind.dnp.dappnode.eth" + } + } + }) + ); + expect(check(dir)).to.deep.equal([]); + }); + + it("formats a report and GitHub annotations with relative paths", () => { + const findings: SpecFinding[] = [ + { + rule: "SPEC-C02", + message: 'Service "app" sets container_name, a, b', + file: path.join(dir, "docker-compose.yml") + }, + { rule: "SPEC-B02", message: "line one\nline two" } + ]; + const report = formatSpecReport(findings, { base: dir }); + expect(report) + .to.include("2 package spec violations") + .and.include("future SDK release"); + expect(report).to.include( + 'SPEC-C02 docker-compose.yml: Service "app" sets container_name' + ); + expect(formatGithubAnnotations(findings, { base: dir })).to.deep.equal([ + `::warning file=docker-compose.yml,title=SPEC-C02 ${specRules["SPEC-C02"].title}::Service "app" sets container_name, a, b`, + `::warning title=SPEC-B02 ${specRules["SPEC-B02"].title}::line one%0Aline two` + ]); + }); + + it("collects findings in warn mode and fails validation only in error mode", async () => { + await initHandler({ dir, yes: true, force: true }); + const compose = yaml.load( + fs.readFileSync(path.join(dir, "docker-compose.yml"), "utf8") + ) as { + services: Record>; + }; + for (const service of Object.values(compose.services)) + service.container_name = "fixed"; + fs.writeFileSync(path.join(dir, "docker-compose.yml"), yaml.dump(compose)); + const packagesToBuildProps = getPackagesToBuildProps({ + allVariants: false, + rootDir: dir, + variantsDirPath: path.join(dir, "package_variants"), + composeFileName: "docker-compose.yml" + }); + const run = async (mode: string | undefined) => { + const original = process.env.DAPPNODE_SDK_SPEC; + if (mode === undefined) delete process.env.DAPPNODE_SDK_SPEC; + else process.env.DAPPNODE_SDK_SPEC = mode; + const specFindings: SpecFinding[] = []; + try { + const error = await new Listr( + [ + getFileValidationTask({ + packagesToBuildProps, + rootDir: dir, + specFindings + }) + ], + { renderer: "silent" } + ) + .run() + .then( + () => null, + e => e + ); + return { error, specFindings }; + } finally { + if (original === undefined) delete process.env.DAPPNODE_SDK_SPEC; + else process.env.DAPPNODE_SDK_SPEC = original; + } + }; + expect(getSpecMode({})).to.equal("warn"); + const warn = await run(undefined); + expect(warn.error).to.equal(null); + expect(rulesOf(warn.specFindings)).to.deep.equal(["SPEC-C02"]); + const strict = await run("error"); + expect(strict.error?.message) + .to.include("SPEC-C02") + .and.include("DAPPNODE_SDK_SPEC=error"); + const off = await run("off"); + expect(off.error).to.equal(null); + expect(off.specFindings).to.deep.equal([]); + }); +});