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

prefetching dependencies refactoring #200

Merged
merged 5 commits into from
Feb 4, 2025

Conversation

slimreaper35
Copy link
Contributor

No description provided.

Copy link

🚀 Preview is available at https://pr-200--konflux-docs.netlify.app

@brunoapimentel
Copy link
Contributor

I think we could greatly simplify this page if we changed the structure so we have a single initial section describing how to se the prefetch-input parameter?, instead of having it repead for each individual package manager. There are some sections that are important to be kept, IMO:

  • Configuring your pip project to build from source (how to generate a requirements-build.txt)
  • Prefetching pip dependencies from custom index servers
  • Generating the rpms.lock.yml file for RPM prefetch
  • Creating a artifacts.lock.yaml file for generic artifact prefetching

All the rest, can probably be inferred and is described in Cachi2's README.

Copy link

github-actions bot commented Jan 3, 2025

🚀 Preview is available at https://pr-200--konflux-docs.netlify.app

Copy link

github-actions bot commented Jan 3, 2025

🚀 Preview is available at https://pr-200--konflux-docs.netlify.app

Copy link

github-actions bot commented Jan 3, 2025

🚀 Preview is available at https://pr-200--konflux-docs.netlify.app

Copy link

github-actions bot commented Jan 3, 2025

🚀 Preview is available at https://pr-200--konflux-docs.netlify.app

Copy link

github-actions bot commented Jan 3, 2025

🚀 Preview is available at https://pr-200--konflux-docs.netlify.app

Copy link

github-actions bot commented Jan 3, 2025

🚀 Preview is available at https://pr-200--konflux-docs.netlify.app

Copy link

github-actions bot commented Jan 8, 2025

🚀 Preview is available at https://pr-200--konflux-docs.netlify.app

Comment on lines 44 to 56
. Configure the hermetic pipeline by adding the following parameters in both `.yaml` files:

+
[source,yaml]
----
pipelineSpec:
params:
...
- name: hermetic
type: string
description: Execute the build with network isolation
default: "true"
----
Copy link
Member

Choose a reason for hiding this comment

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

This belongs on the hermetic page, but we can refer to that page.

This was originally split into two pages because each can be done independently. We can still increase the accuracy of the SBOM by prefetching even if the build isn't done in a hermetic mode, right?

Comment on lines 65 to 67
value: '{"type": "<package_manager>", "path": "."}' <1>
----
NOTE: The prefetch-input parameter specifies the path to the directory of the project. In this example, the `.` indicates the repository root. If you have multiple directories, you can provide the path to those directories in the JSON array format: `[{"type": "<package_manager>", "path": "."}, {"type": "<package_manager>", "path": "subpath/to/the/other/directory"}]`.
Copy link
Member

Choose a reason for hiding this comment

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

You have the inline <1> here but are using a NOTE comment instead.

Comment on lines 65 to 67
value: '{"type": "<package_manager>", "path": "."}' <1>
----
NOTE: The prefetch-input parameter specifies the path to the directory of the project. In this example, the `.` indicates the repository root. If you have multiple directories, you can provide the path to those directories in the JSON array format: `[{"type": "<package_manager>", "path": "."}, {"type": "<package_manager>", "path": "subpath/to/the/other/directory"}]`.
Copy link
Member

Choose a reason for hiding this comment

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

It is also possible to use a tekton array, specifying a different package manager configuration per line. Is that more usable? Should we suggest that or at least mention it (it is definitely harder to give an inline example like you did here)?

- name: dev-package-managers
value: "true"
----
NOTE: You won't find `dev-package-managers` as a param on the `prefetch-dependencies` task. You have to add it, and set it to true. This is because Cachi2 hasn't declared stable support for the feature yet. Use it at your own risk.
Copy link
Member

Choose a reason for hiding this comment

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

If we think that many Konflux users will leverage this, should we start exposing it to reduce the amount of manual work that needs to be done?

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 section should temporarily be nested under the RPM section only (we only need it for RPMs). And then I think the next step is to remove RPM from dev-package-managers.


== Verification
* From the {ProductName} *Applications* view, go to *Activity > Pipeline runs*.
** Go to the pipeline run with *Build* in the *Type* column and confirm that the `prefetch-dependencies` stage displays a green checkmark. This indicates that the build process successfully fetched all dependencies.
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible for the task to show green if input has been provided but not prefetched (i.e. bad configuration which doesn't correspond to a package manager or skipped fetching)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the color of the prefetch task, if the hermetic pipelineSpec param is set to false, but prefetch-input is not empty ?

Copy link
Member

Choose a reason for hiding this comment

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

Prefetch can work on its own regardless of the hermetic parameter. If successful, then I think it should indicate a green check mark if the process worked.

@brunoapimentel
Copy link
Contributor

I still think we should get completely rid of the individual package manager sections, unless they're really necessary. I don't think the "Prerequisites" section adds much valeu, and the rest is essentially repetition.

This is a rough suggestion on how I see the sections (note that after "Troubleshooting, all sections are in-depth/specific info)

  • Title
  • Procedure
  • Verification
  • Troubleshooting
  • Building Pip wheels from source
  • Configuring the repository for RPM prefetching
  • Enabling the prefetching of binary files (pip and bundler)
  • Prefetching generic artifacts

@arewm
Copy link
Member

arewm commented Jan 14, 2025

I like the idea of having some summary/representation of the prefetch configuration in this document but I don't think that we should repeat everything in the Cachi2 documentation. What would you think about having a table with a sample input to the prefetch dependency task for each package manager. We can then have sections later with specific things that need to be done (like enabling the dev package managers, binary file prefetchign, etc.). The table can then point users to the specific documentation for the prefetch elsewhere (i.e. cachi2 docs) and/or the the sections later on the page.

@slimreaper35
Copy link
Contributor Author

I am back. Let's finish this off 🚀

I don't think that we should repeat everything in the Cachi2 documentation.

I believe we all agree on that.

The table can then point users to the specific documentation for the prefetch elsewhere (i.e. cachi2 docs)

👍

This is a rough suggestion on how I see the sections (note that after "Troubleshooting, all sections are in-depth/specific info)

Currently, there is a Title, Procedure, Verification, and Troubleshooting. I thought we could have something like Tutorials where we would add those specific things.

Copy link

🚀 Preview is available at https://pr-200--konflux-docs.netlify.app

@slimreaper35
Copy link
Contributor Author

Let's not make the scope of this PR too big. I basically moved duplicate paragraphs for the Procedure, Verification, and Troubleshooting sections to the top of the page.

Copy link
Member

@arewm arewm left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. I think it looks like an improvement. We can iterate with future PRs to improve further.

Comment on lines -516 to -519
== Additional resources

* To troubleshoot any issues you might experience when you enable prefetch builds for `pip` or `pip` with source dependencies, see link:https://github.com/containerbuildsystem/cachi2/blob/main/docs/pip.md#troubleshooting[Troubleshooting].
* For more information about Cachi2, see link:https://github.com/containerbuildsystem/cachi2/blob/main/docs/usage.md[Cachi2].
Copy link
Member

Choose a reason for hiding this comment

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

Do we not want to link to these anymore?

Copy link
Contributor Author

@slimreaper35 slimreaper35 Feb 4, 2025

Choose a reason for hiding this comment

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

Copy link

github-actions bot commented Feb 4, 2025

🚀 Preview is available at https://pr-200--konflux-docs.netlify.app

Copy link
Member

@arewm arewm left a comment

Choose a reason for hiding this comment

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

Thanks for these changes!

@arewm arewm merged commit 3e45341 into konflux-ci:main Feb 4, 2025
3 checks passed
@slimreaper35
Copy link
Contributor Author

NP! Thank you for reviewing it.

@slimreaper35 slimreaper35 deleted the prefetching branch February 4, 2025 14:32
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.

3 participants