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

Support for prefetching RPMs [PoC] #502

Merged
merged 5 commits into from
Apr 12, 2024

Conversation

onosek
Copy link
Contributor

@onosek onosek commented Mar 19, 2024

This is the proof of concept of RPM prefetching functionality.
Design concept: #465

A new rpm package manager was added as a development version so --dev-package-managers flag needs to be activated.

  • fetch-deps command will read the rpms lockfile and download listed packages and sources into the output directory structure. Also SBOM file will be generated (bom.json).
  • inject-files command will create repofile for each arch and create repository metadata out of its RPMs for every repoid dir.

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:

@onosek
Copy link
Contributor Author

onosek commented Mar 19, 2024

  • Missing unittests
  • Contains some # NOTE comments - they will be removed after clarification.
  • Input file source/rpms.lock.yaml won't be part of the merge. It serves just for your testing:
    cachi2 fetch-deps --log-level DEBUG --dev-package-managers rpm -source source --output output
    cachi2 inject-files --log-level DEBUG output

@onosek onosek force-pushed the rpm_prefetch_poc branch from 3e5ac1a to 5165784 Compare March 22, 2024 00:34
Copy link
Member

@eskultety eskultety left a comment

Choose a reason for hiding this comment

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

Some high-level comments that I could spot right away, will a deeper review once this is broken into more commits.

cachi2/core/package_managers/rpm/base.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/rpm/main.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/rpm/main.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/rpm/main.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/rpm/main.py Outdated Show resolved Hide resolved
source/rpms.lock.yaml Outdated Show resolved Hide resolved
source/rpms.lock.yaml Outdated Show resolved Hide resolved
cachi2/core/package_managers/rpm/redhat.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/rpm/main.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/rpm/main.py Outdated Show resolved Hide resolved
@onosek onosek force-pushed the rpm_prefetch_poc branch from 5165784 to 8722b59 Compare April 3, 2024 02:19
@onosek onosek force-pushed the rpm_prefetch_poc branch from 8722b59 to abe11c7 Compare April 3, 2024 23:11
@onosek onosek requested a review from eskultety April 3, 2024 23:19
Copy link
Member

@eskultety eskultety left a comment

Choose a reason for hiding this comment

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

None of the commits passes unit tests which causes the commits to not be bisectable. Please make sure the unit test suite passes after each commit, FWIW adding linter checks in there is a benefit - I suggest running git rebase main -x 'tox -e black,flake8,isort,bandit,mypy,py39' before pushing your changes to the branch.

The fact that tests currently don't pass for any of the commits is partially due to suboptimal logical structuring of them, IOW, I would expect a logical structure similar (doesn't need to be identical) to the following:

  1. Model and interface class declaration with almost no real logic (i.e. using pass in for the interface methods)
  2. fetch_rpm_source minimal implementation and base model validations
  3. Implementing RPM download and verification
  4. Implementing SBOM generation
  5. Implementing inject_files

Unit tests should be part of each commit, reflecting the changes - you already got an implementation proposal on those so those should be incorporated in the next revision.

Lastly, the commit message subjects and bodies should not be tied to the name of the PR, but to the actual logic being implemented, i.e. an example for the above structure could be:

  1. rpm: Introduce base models for RPM lockfile
  2. rpm: Introduce fetch_rpm_sources (the commit message would mention model validation)
  3. rpm: Implement RPM package downloads and checksum validation
  4. rpm: Implement SBOM generation
  5. rpm: Implement inject-files support

cachi2/core/package_managers/rpm/main.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/rpm/main.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/rpm/main.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/rpm/main.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/rpm/main.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/rpm/main.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/rpm/main.py Outdated Show resolved Hide resolved
cachi2/core/resolver.py Outdated Show resolved Hide resolved
cachi2/interface/cli.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/rpm/redhat.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/rpm/main.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/rpm/redhat.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/rpm/main.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/rpm/redhat.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/rpm/redhat.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/rpm/redhat.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/rpm/redhat.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/rpm/redhat.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/rpm/redhat.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/rpm/redhat.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/rpm/redhat.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/rpm/redhat.py Outdated Show resolved Hide resolved
@onosek onosek force-pushed the rpm_prefetch_poc branch from abe11c7 to 5a27320 Compare April 10, 2024 01:50
@tkdchen
Copy link
Contributor

tkdchen commented Apr 10, 2024

/ok-to-test

Copy link
Member

@eskultety eskultety left a comment

Choose a reason for hiding this comment

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

Really mostly minor nitpicks related to wording of log messages, but other than that, this is working, doesn't crash in between commits; the commits are structured well. Anything else can be done in follow up patches as optimization/refactoring.Overall, this is looking really good, just waiting for the unit tests.

cachi2/core/package_managers/rpm/main.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/rpm/main.py Show resolved Hide resolved
cachi2/core/package_managers/rpm/main.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/rpm/main.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/rpm/main.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/rpm/main.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/rpm/main.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/rpm/main.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/rpm/main.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/rpm/main.py Outdated Show resolved Hide resolved
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.

Leaving my round of reviews, they're mostly nitpicks and improvement ideas (all properly identified). Please, don't bother with them too much, let's focus on the few comments that are not nitpicks.

Congrats on the work here @onosek @eskultety. The code is looking much more organized and concise than the last time I reviewed it 💪.

cachi2/core/package_managers/rpm/redhat.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/rpm/redhat.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/rpm/redhat.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/rpm/main.py Show resolved Hide resolved
cachi2/core/package_managers/rpm/main.py Show resolved Hide resolved
cachi2/core/resolver.py Show resolved Hide resolved
@onosek onosek force-pushed the rpm_prefetch_poc branch from 5a27320 to cd9933a Compare April 11, 2024 02:52
cachi2/core/package_managers/rpm/main.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/rpm/main.py Show resolved Hide resolved
cachi2/core/package_managers/rpm/main.py Show resolved Hide resolved
cachi2/core/package_managers/rpm/redhat.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/rpm/main.py Outdated Show resolved Hide resolved
@brunoapimentel brunoapimentel mentioned this pull request Apr 11, 2024
4 tasks
This is the proof of concept of RPM prefetching functionality.
Design concept:
containerbuildsystem#465

A new rpm package manager was added as a development version
so `--dev-package-managers` flag needs to be activated.
* `fetch-deps` command will read the rpms lockfile and download listed
packages and sources into the output directory structure. Also SBOM
file will be generated (bom.json).
* `inject-files` command will create repofile for each arch and create
repository metadata out of its RPMs for every repoid dir.

The whole prefetching PoC functionality is split into a few commits:
This first commit creates a new development rpm package manager
with `fetch-deps` method (without functionality) applicable to it.
It introduces the model of the RPM lockfile too.

Signed-off-by: Ondřej Nosek <[email protected]>
@onosek onosek force-pushed the rpm_prefetch_poc branch from cd9933a to 7c7b2f7 Compare April 11, 2024 23:39
onosek added 2 commits April 12, 2024 13:00
Add `fetch-deps` functionality for the rpm package manager.
RedHat lockfile format is validated and processed.

Signed-off-by: Ondřej Nosek <[email protected]>
All packages and sources in the lockfile are downloaded into
an output structure and then verified per checksums. The output
structure is hierarchically organized into arch and repoid
subdirectories.

Signed-off-by: Ondřej Nosek <[email protected]>
@onosek onosek force-pushed the rpm_prefetch_poc branch from 7c7b2f7 to b73d616 Compare April 12, 2024 11:05
Copy link
Member

@eskultety eskultety left a comment

Choose a reason for hiding this comment

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

One last nitpick typo from me, but I think we're good to go -> Approve.

tests/unit/package_managers/test_rpm.py Show resolved Hide resolved
tests/unit/package_managers/test_rpm.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/rpm/main.py Show resolved Hide resolved
@onosek onosek force-pushed the rpm_prefetch_poc branch from b73d616 to e73aacc Compare April 12, 2024 13:12
onosek added 2 commits April 12, 2024 16:14
During the `fetch-deps` command, SBOM file is generated (bom.json).
SBOM file contains metadata about downloaded packages.
Metadata is taken from (S)RPM file headers and from the lockfile
itself.

Signed-off-by: Ondřej Nosek <[email protected]>
`inject-files` do extra steps for rpm package manager:

* Repository metadata is created for every repoid directory.
Metadata is created by executing the `createrepo_c` command
on the particular directory.

* Generate 'cachi2.repo' files for all arches and save them
into their own subdirectory 'repos.d'.

Signed-off-by: Ondřej Nosek <[email protected]>
@onosek onosek force-pushed the rpm_prefetch_poc branch from e73aacc to 38ede91 Compare April 12, 2024 14:17
@eskultety eskultety added this pull request to the merge queue Apr 12, 2024
Merged via the queue into containerbuildsystem:main with commit 6597967 Apr 12, 2024
15 checks passed
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.

5 participants