-
Notifications
You must be signed in to change notification settings - Fork 41
Packit: initial enablement #30
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
Conversation
|
@jankaluza @Luap99 PTAL. |
mtrmac
left a comment
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.
Just a brief skim, I don’t know what we want/need to do via Packit, and how/if at all it interacts with other downstream packaging.
| ensure pkg/config/containers.conf runtime \"crun\" | ||
| ensure pkg/config/containers.conf log_driver \"journald\" | ||
| ensure common/pkg/config/containers.conf runtime \"crun\" | ||
| ensure common/pkg/config/containers.conf log_driver \"journald\" |
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.
There was some … discussion in #4 about how packaging happens. Does this risk interfering with downstream RPMs?
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.
This has been happening already in containers-common rpm that was done from the c/common repo and shipped to Fedora for quite sometime now. Only the path was updated to match monorepo. The final list of files installed is not being changed (after I remove the additional docs stuff that you commented about)
|
btw, The Common: Test is fixed in main, so rebase should help. |
586f3b9 to
1e62679
Compare
|
Packit jobs failed. @containers/packit-build please check. |
|
Ready for review after addressing @mtrmac 's initial comments. |
Luap99
left a comment
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.
Can you remove the common/.packit.yaml file then? I assume then git would show this file as rename which likely helps for reviewers.
Let me check. I think .packit.yaml is expected in top-level. |
Yes that is fine, what I am saying is you should remove |
Ah ok. My bad. Doing it. |
|
Done. Thanks, and sorry for my misread earlier. |
Signed-off-by: Lokesh Mandvekar <[email protected]>
Luap99
left a comment
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.
LGTM, if the chnage is intentional. Though I don't really have tried to understand most of the changes.
| # Ignore CentOS Stream for now | ||
| - job: propose_downstream | ||
| trigger: release | ||
| trigger: ignore |
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.
I assume that change is intentional?
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.
yes, this one is intentional. We're not doing centos stream official updates via packit atm.
Luap99
left a comment
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.
LGTM
No description provided.