docs(generic): Convert generic-fetcher ADR to a design document - #1720
docs(generic): Convert generic-fetcher ADR to a design document#1720hitshiroya wants to merge 8 commits into
Conversation
📝 WalkthroughWalkthroughThe generic fetcher ADR is replaced by a design document. The document defines the lockfile format, artifact validation, direct fetching, authentication, output layout, build integration, and CycloneDX SBOM representation. Contribution guidance now requests a Design Document. Markdownlint and MkDocs no longer exclude the ADR path. Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: efe83fd8-eb4f-4afe-8b2c-f24329416f47
📒 Files selected for processing (5)
.markdownlint-cli2.yamlCONTRIBUTING.mddocs/adr/0001-add-generic-fetcher.mddocs/design/generic-fetcher.mdmkdocs.yml
💤 Files with no reviewable changes (3)
- .markdownlint-cli2.yaml
- mkdocs.yml
- docs/adr/0001-add-generic-fetcher.md
dd8a9c2 to
688b57d
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 69eb2ae9-78cc-4f7c-aa7a-1e4655e7ddcc
📒 Files selected for processing (2)
CONTRIBUTING.mddocs/design/generic-fetcher.md
| **In scope:** | ||
|
|
||
| - Hermeto package type `generic` | ||
| - Custom lockfile `artifacts.lock.yaml` (versioned via `metadata.version`) | ||
| - Download of arbitrary URL artifacts, checksum verification, and placement | ||
| under `{hermeto-output-dir}/deps/generic` | ||
| - SBOM components with `pkg:generic` PURLs and CycloneDX `externalReferences` |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline hermeto/core/package_managers/generic/models.py --items all
ast-grep outline hermeto/core/package_managers/generic/main.py --items all
rg -n -C 6 \
'Maven|class .*Artifact|auth|bearer|Basic|netrc|username|password|\$\{VAR\}' \
hermeto/core/package_managers/generic tests -g '*.py'Repository: hermetoproject/hermeto
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- design headings and Maven references ---'
rg -n '^#{1,6} |Maven|maven|pkg:generic|pkg:maven|artifacts.lock.yaml|authentication|auth' docs/design/generic-fetcher.md
printf '%s\n' '--- relevant design sections ---'
sed -n '1,180p' docs/design/generic-fetcher.md
sed -n '180,380p' docs/design/generic-fetcher.md
sed -n '380,560p' docs/design/generic-fetcher.md
printf '%s\n' '--- generic tests and fixtures ---'
git ls-files 'tests/*' 'tests/**/*' | rg 'generic|artifact' | head -80
rg -n -C 3 'LockfileArtifactMaven|group_id|artifact_id|pkg:maven|GenericLockfileV[12]|artifacts.lock.yaml' tests/unit/package_managers/generic tests/integration 2>/dev/null | head -240Repository: hermetoproject/hermeto
Length of output: 50380
Document Maven artifact support
The branch includes LockfileArtifactMaven and LockfileArtifactMavenV2, with an integration scenario and pkg:maven SBOM output. Extend this design to cover Maven lockfile fields, URL and filename resolution, authentication, output placement, and SBOM representation.
…cture The docs/adr/ directory held a single ADR that overlaps with the design documents already tracked under docs/design/. Moving it there avoids maintaining two separate locations for design-level content and makes it easier for contributors to find all relevant documents in one place. - Moved docs/adr/0001-add-generic-fetcher.md to docs/design/generic-fetcher.md - Removed reference of adr/ from mkdocs.yml - Removed reference of adr/ from .markdownlint-cli2.yaml - Updated CONTRIBUTING.md with the design doc reference Signed-off-by: HitShiroya <hitp363530@gmail.com> Assisted-by: Cursor
This commit is intentional. Since a final decision had not been made about whether to remove the directory, this commit was added separately to make things easier. As the ADR document has now been moved to design/ to provide a consistent documentation experience, the adr/ directory was empty and no longer served any purpose. Hence, it has been deleted. Signed-off-by: HitShiroya <hitp363530@gmail.com> Assisted-by: Cursor
The ADR moved to docs/design/ was a minimal record of the original decision. This expands it into a full design document following the package-manager template, covering all sections needed to understand and maintain the generic backend. Sections added or expanded: - Developer workflow with example hermeto invocation - How the package manager works (registry model, identity, resolution) - Design scope (in/out of scope) with references to related docs - Dependency list format with field-level documentation and examples - Checksum generation and validation approach - Fetching content and project directory structure - Build environment configuration and integration - SBOM components with CycloneDX externalReferences example - Implementation notes and current limitations Signed-off-by: HitShiroya <hitp363530@gmail.com> Assisted-by: Cursor
- Fix grammar: "an Design Document" -> "a Design Document" in CONTRIBUTING.md - Improve code comment clarity in artifacts.lock.yaml example - Reword SBOM components paragraph for accuracy Signed-off-by: HitShiroya <hitp363530@gmail.com> Assisted-by: Cursor
Currently generic-auth is a standalone doc. decision was to merge auth doc into generic-fetcher design doc, which will make it easier for user to follow the generic-fetcher along with it's authentication flow. - auth section along with example accomodate under Dep List Format = auth-background section accomodate under Fetching Content section Signed-off-by: HitShiroya <hitp363530@gmail.com> Assisted-by: Cursor
Sections merged from generic-auth.md: - aiohttp integration detail into -> Integrating with aiohttp - End-to-end authenticated fetch example into -> Developer Workflow Signed-off-by: HitShiroya <hitp363530@gmail.com> Assisted-by: Cursor
Documents three auth extensions that are out of scope for the current implementation but worth tracking for future contributors: - Input JSON authentication for backends with single registries - AWS Signature Version 4 for S3-compatible artifact sources - Other IANA-defined HTTP auth schemes Merged from the Potential Future Extensions section of generic-auth.md. Signed-off-by: HitShiroya <hitp363530@gmail.com> Assisted-by: Cursor
- Clarify that the auth field is not needed for public resources or HTTP Basic auth (covered by .netrc) - Broaden "bearer token authentication" to "token-based authentication" to avoid constraining future auth types such as AWS SigV4 - Drop redundant "optional" from the developer workflow auth note since "for private resources" already conveys conditionality Signed-off-by: HitShiroya <hitp363530@gmail.com> Assisted-by: Cursor
4d0ffe6 to
1456236
Compare
a-ovchinnikov
left a comment
a-ovchinnikov
left a comment
There was a problem hiding this comment.
I have reviewed the first half of the PR (up to the commit that merges generic-auth) and I got quite a few questions, please find them inline.
| ### Developer Workflow | ||
|
|
||
| ### Lockfile format | ||
| 1. **Prerequisites**: No external package-manager CLI is required. Developers |
There was a problem hiding this comment.
Does this have to be a list? I think it would benefit a user if it were transformed into a regular text (I am open to counter-arguments).
There was a problem hiding this comment.
Paragraph do work, but I have followed the design doc template to maintain consistency.
https://github.com/hermetoproject/hermeto/blob/main/docs/design/package-manager-template.md
| A new package manager for generic artifacts must be introduced. This package manager utilizes a custom | ||
| lockfile based on which it will download files, save them into a requested location, and verify checksums. | ||
| Below is a more detailed overview of the implementation. | ||
| ### Developer Workflow |
There was a problem hiding this comment.
I am sorry, why is this section called "Developer Workflow"? Which developer did you mean, Hermeto developer?
There was a problem hiding this comment.
It’s based on the design doc template. Here, “developer workflow” refers to developers who are using the package manager.
https://github.com/hermetoproject/hermeto/blob/main/docs/design/package-manager-template.md#developer-workflow
| 2. **Adding dependencies**: Add an entry under `artifacts` with a | ||
| `download_url`, a required `checksum`, and an optional `filename`. | ||
| 3. **Dependency management**: Developers maintain the lockfile by hand (or with | ||
| their own tooling). Hermeto does not resolve or update dependencies. |
There was a problem hiding this comment.
Rgith now there is quite a lot of focus on manual opertations. WDYT about shifting it? E.g. like this: "" Developers maintain the lockfile by themselves, Hermeto does not resolve or update dependencies by its own."
There was a problem hiding this comment.
Good idea. Short and simple would be better. I thought the same while writing it. But again, if we look at the design doc template, it has all these points listed. Converting them into a paragraph would not be consistent with the template, so I followed the same pattern. But we can think about this.
|
|
||
| ### How the Package Manager Works | ||
|
|
||
| - **Registry/repository model**: There is no single registry. Each artifact is |
There was a problem hiding this comment.
If there is no registry I'd say there is no point in naming a subsection "Registry". And here again I do not understand what is the purpose of a list?
There was a problem hiding this comment.
Makes sense. I followed the same pattern as the template.
https://github.com/hermetoproject/hermeto/blob/main/docs/design/package-manager-template.md#how-the-package-manager-works
| for those produces less accurate SBOM components (see | ||
| [docs/generic.md](../generic.md)) | ||
| - Dependency resolution or lockfile generation by Hermeto | ||
| - Credential acquisition flows (OAuth2, OIDC); see |
There was a problem hiding this comment.
Are you sure this is out of scope and not "tracked separately"? I generally have doubts about the helpfulness of In scope/out of scope sections, at least the way they are presented now. In scope is redundant and to me is overly specific: L53-L55 introduce the scope reasonably well, then L57-L64 repeat the same, but also with implementation details like {hermeto-output-dir}/deps/generic or "CycloneDX externalReferences of type distribution" (and since you mentioned CyDX by name what about SPDX?).
| Developer project (typical): | ||
|
|
||
| Artifacts fetched with the generic fetcher will all be recorded in the SBOM hermeto produces. Given the inability to derive | ||
| any extra information about these files beyond a download location and a filename, these files will always be recorded | ||
| as SBOM components with purl of type generic. | ||
| ``` | ||
| project.git/ | ||
| ├── artifacts.lock.yaml | ||
| └── ... | ||
| ``` | ||
|
|
||
| Hermeto output after `fetch-deps`: | ||
|
|
||
| ``` | ||
| hermeto-output/ | ||
| └── deps/ | ||
| └── generic/ | ||
| └── <filename or URL-derived name> | ||
| ``` | ||
|
|
||
| #### File Formats and Metadata | ||
|
|
||
| - **Package file formats**: Opaque; any file reachable by URL | ||
| - **Naming conventions**: Prefer explicit `filename`; otherwise derive from URL. | ||
| Overlapping destination paths is an error. | ||
| - **Version handling**: No version directory layout; each artifact is a single | ||
| file path under `deps/generic/` | ||
|
|
||
| ### Build Environment Config | ||
|
|
||
| The generic fetcher does not configure an external package manager. Builds | ||
| consume files by path from Hermeto's output directory. | ||
|
|
||
| #### Environment Variables | ||
|
|
||
| No package-manager-specific environment variables are required for consumption. | ||
| Builds typically mount the Hermeto output directory and reference files | ||
| directly. | ||
|
|
||
| | Variable Name | Purpose | Example Value | Required | | ||
| |---------------|---------|---------------|----------| | ||
| | — | No dedicated env vars for consuming generic artifacts | — | — | |
There was a problem hiding this comment.
I am sorry, I do not understand what does the highlighted section add to the document? It looks like a repetition of trivial facts which could get derived from the first paragraph. I think I am missing something here.
There was a problem hiding this comment.
Yes, I agree the paragraph above is sufficient to convey the information. I will remove the table.
|
|
||
| Additionally, the SBOM component contains [externalReferences] of type | ||
| `distribution` to indicate the URL used to download the file, for easier | ||
| handling by tools that process the SBOM. |
There was a problem hiding this comment.
How does addition of externalReferences make it easier for tools to process a SBOM?
There was a problem hiding this comment.
I read this in the ADR document, and my understanding is that it’s a generic fetcher that doesn’t have a package manager. Everything is maintained by the author, and dependencies can be downloaded from the internet as long as they have a valid URL and checksum mentioned in the lock file. externalReferences can help ensure where the dependency was downloaded from or processed.
| The generic fetcher enables users to fetch arbitrary files with Hermeto and have | ||
| them accounted for in the SBOM. It is a fully supported package type (`generic`, | ||
| not `x-generic`). |
There was a problem hiding this comment.
I am sorry, I don't understand how any of this is an "Implementation Note". Wasn't "The generic fetcher enables users to fetch arbitrary files" said a few times before? Also what is the importance of stating that generic is fully supported? This is not an implementation detail, it is a fact of development process. Please tell me what I am missing here.
There was a problem hiding this comment.
According to the Design Doc Template, this section provides information about the maturity level of the package manager. If it’s experimental, we use x-pkg-manager. This section conveys that information. Hope this helps.
There was a problem hiding this comment.
Please apply suggested changes to commits that introduce pieces which need improvements. This entire commit should be split between the first commit and the major rewrite commit.
There was a problem hiding this comment.
Most of the changes are related to formatting and minor wording. The formatting follows the Design Doc Template for consistency. Let’s discuss the final formatting, and accordingly I can make the changes. How does that sound?
There was a problem hiding this comment.
02631cd: Commit message:
decision was to merge auth doc into generic-fetcher design doc
Could you please add a pointer to a public record of this decision? All I could find was a mention of merging as an option in the issue this PR resolves.
2 participants
What
Converts
docs/adr/0001-add-generic-fetcher.mdinto a complete design document atdocs/design/generic-fetcher.md, covering all sections defined in the package-manager template: developer workflow, lockfile format, fetching content, build environment config, SBOM components, and current limitations. References inCONTRIBUTING.md,mkdocs.yml, and.markdownlint-cli2.yamlare updated accordingly.Why
The original ADR was a minimal decision record for contributors to understand, extend, or maintain the generic backend. A complete design document brings it in line with the other backends (Bundler, RPM) and gives reviewers and future contributors a single authoritative reference for all design decisions.
closes #1672