Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

generic fetcher: Add usage docs and a ADR #718

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kosciCZ
Copy link
Contributor

@kosciCZ kosciCZ commented Nov 1, 2024

Add documentation on how to use the generic fetcher and also an ADR to help move out of the experimental phase.

I've tried to follow the CONTRIBUTING.md guidelines, but since I did not see any previous ADR, I just tried to come up with something. Feel free to give more direction on how you want the docs to look like. Thanks!

Maintainers will complete the following section

  • Commit messages are descriptive enough
  • Code coverage from testing does not decrease and new code is covered
  • Docs updated (if applicable)
  • Docs links in the code are still valid (if docs were updated)

Note: if the contribution is external (not from an organization member), the CI
pipeline will not run automatically. After verifying that the CI is safe to run:

@brunoapimentel
Copy link
Contributor

/ok-to-test

@brunoapimentel
Copy link
Contributor

brunoapimentel commented Nov 1, 2024

We also add a tiny introduction of each package manager here: https://github.com/containerbuildsystem/cachi2/blob/main/README.md#package-managers. We should have one for generic.

@kosciCZ kosciCZ force-pushed the generic-fetcher-docs branch 2 times, most recently from 5cb71d3 to e5286c0 Compare November 4, 2024 10:05
Add documentation on how to use the generic fetcher and also an ADR
to help move out of the experimental phase.

Signed-off-by: Jan Koscielniak <[email protected]>
Copy link
Contributor

@brunoapimentel brunoapimentel left a comment

Choose a reason for hiding this comment

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

This LGTM in the overall.

I think we should split the ADR to a separate commit, or even have it in a separate PR. The reason is that I think we'll merge the docs very quickly, but the ADR also implies making the package manager officially supported, so it'll bring a longer discussion (and we could use the same PR to also remove it from dev-package-managers).

Specified as a dictionary of checksum algorithms and their values. At least one cachi2-verifiable checksum must be provided
to ensure at least some degree of confidence in the identity of the artifact.

#### target (optional)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a good moment to bring back the discussion about the name of this property. IIRC, somebody suggested it to be called simply filename, so that it makes its goal clearer.

@@ -237,6 +241,18 @@ Both files must be present in the source repository so you should check them int

See [docs/bundler.md](docs/bundler.md) for more details.

### generic fetcher

Generic fetcher is a way for Cachi2 to support pre-fetching arbitrary files that don't fit into other package managers.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: I think we decided to call it "prefetch" (without the hyphen)

"components": [
{
"name": "granite-model-1.safetensors",
"purl": "pkg:generic/granite-model-1.safetensors?checksums=sha256:d16bf783cb6670f7f692ad7d6885ab957c63cfc1b9649bc4a3ba1cfbdfd5230c&download_url=https://huggingface.co/instructlab/granite-7b-lab/resolve/main/model-00001-of-00003.safetensors",
Copy link
Contributor

Choose a reason for hiding this comment

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

Another discussion is whether we want to report the checksum as part of the output purl. I believe none of the existing package managers do it.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is also a leftover discussion from a previous PR: in case we do want to report them as part of the output purl, do we want to only report the ones that were validated?

Copy link
Contributor

Choose a reason for hiding this comment

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

Another discussion is whether we want to report the checksum as part of the output purl. I believe none of the existing package managers do it.

Correction: we do report checksums in purls for file dependencies for package managers such as npm, yarn and pip.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants