-
Notifications
You must be signed in to change notification settings - Fork 42
packit: add initial support #1003
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
base: main
Are you sure you want to change the base?
Conversation
|
Failed to load packit config file: For more info, please check out the documentation or contact the Packit team. You can also use our CLI command |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request adds an initial packit configuration for automating downstream packaging. The configuration has a few issues that could prevent it from working as intended. I've identified a couple of critical issues related to the spec file handling and job triggers that need to be addressed. I've also included some suggestions to improve the configuration's maintainability and use packit's features more effectively. My feedback focuses on establishing a clear source of truth for the spec file and ensuring the job triggers are correctly configured for the desired workflow.
.packit.yaml
Outdated
| post-upstream-clone: | ||
| - wget https://src.fedoraproject.org/rpms/rust-bootupd/blob/rawhide/f/rust-bootupd.spec |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The post-upstream-clone action downloads the spec file from the Fedora rawhide branch. This will overwrite any spec file present in this repository, making local changes to the spec file ineffective. This action should be removed to use the spec file from this repository as the source of truth. This also makes the configuration simpler and avoids a circular dependency where the spec file is fetched from downstream and then proposed back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, wow. Yes I think this is probably right.
Though honestly I still disagree with packit's focus on maintaining spec files in upstream by default. It's convenient for projects that are mainly Fedora-derivative only. There's just a deep fundamental conflict with distros owning their build rules.
IOW, yes we can probably drop this if we're maintaining the spec upstream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I should use contrib/packaging/bootupd.spec instead.
.packit.yaml
Outdated
| - fedora-all | ||
|
|
||
| - job: bodhi_update | ||
| trigger: commit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
b3c0462 to
7b45c08
Compare
7a312b8 to
a6b2612
Compare
a6b2612 to
4872c13
Compare
This is part of Fedora BootLoaderUpdatesPhase1: https://fedoraproject.org/wiki/Changes/BootLoaderUpdatesPhase1
Extract duplicated ESP mounting, validation, and copying logic from install() and package_mode_copy_to_boot_impl() into shared helper function to eliminate dupe code.
Addressing review: add unit test that installs shim into a container and ensures that the files are properly setup in the right place
4872c13 to
2a9b4be
Compare
Run `bootupctl backend copy-to-boot` in posttans for F44+
2a9b4be to
62e1ddd
Compare
81f9ef7 to
661ffa8
Compare
661ffa8 to
48d7a78
Compare
This config is setup to target downstream job automation.