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.

Sign up for GitHub

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

Jump to bottom

RPM: Remove crio #524

Merged
merged 2 commits into from
Sep 5, 2024
Merged

RPM: Remove crio #524

merged 2 commits into from
Sep 5, 2024

Conversation

lsm5
Copy link
Member

@lsm5 lsm5 commented Sep 2, 2024

No description provided.

lsm5 added 2 commits September 2, 2024 18:34
@lsm5
RPM: do not create cri-o dirs
4361819 
cri-o dirs will be owned by the cri-o package

Fixes: containers#517

Signed-off-by: Lokesh Mandvekar <[email protected]>
@lsm5
RPM: cleanup changelog conditionals
8abac78 
All of our official downstream envs that build from this spec file
support autochangelog so we don't need these conditionals.

CentOS Stream 9 builds with this spec file are only on
the podman-next COPR, so removing these conditionals is no big deal.

Signed-off-by: Lokesh Mandvekar <[email protected]>
@lsm5
Copy link
Member Author

lsm5 commented Sep 2, 2024

@saschagrunert @haircommander do we need the Packaging for Fedora ... cirrus jobs if we have packit?

@lsm5 lsm5 changed the title Remove crio RPM: Remove crio Sep 2, 2024
@lsm5 lsm5 linked an issue Sep 2, 2024 that may be closed by this pull request
RPM installs to /usr/libexec/crio/conmon which should be owned by CRI-O #517
Closed
@saschagrunert
Copy link
Member

@lsm5 I don't think so.

mdbooth
mdbooth reviewed Sep 2, 2024
View reviewed changes
Copy link

@mdbooth mdbooth left a comment

Choose a reason for hiding this comment

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

IIUC this meets the packaging requirements, namely not to touch %{_libexecdir}/crio, which is owned by crio. If the podman tests still pass, which from the discussion we seem confident they will, this looks good to me.

@lsm5
Copy link
Member Author

lsm5 commented Sep 2, 2024

IIUC this meets the packaging requirements, namely not to touch %{_libexecdir}/crio, which is owned by crio. If the podman tests still pass, which from the discussion we seem confident they will, this looks good to me.

Could you please point to the exact podman tests in this repo? We can and probably should replace any Cirrus tests with TMT.

@lsm5 lsm5 marked this pull request as ready for review September 2, 2024 13:45
@lsm5
Copy link
Member Author

lsm5 commented Sep 2, 2024

PTAL.

@mdbooth
Copy link

mdbooth commented Sep 2, 2024

IIUC this meets the packaging requirements, namely not to touch %{_libexecdir}/crio, which is owned by crio. If the podman tests still pass, which from the discussion we seem confident they will, this looks good to me.

Could you please point to the exact podman tests in this repo? We can and probably should replace any Cirrus tests with TMT.

Sorry, this was just my hand-wavy assumption that they must exist somewhere and will be executed here. I believe the primary concern about this change is that it could break podman (but we don't believe it will because podman is using a different conmon binary).

I did notice that there appear to be some podman tests in the Fedora repo here: https://src.fedoraproject.org/rpms/conmon/blob/rawhide/f/tests. I don't know what they do, though.

@lsm5
Copy link
Member Author

lsm5 commented Sep 2, 2024

IIUC this meets the packaging requirements, namely not to touch %{_libexecdir}/crio, which is owned by crio. If the podman tests still pass, which from the discussion we seem confident they will, this looks good to me.

Could you please point to the exact podman tests in this repo? We can and probably should replace any Cirrus tests with TMT.

Sorry, this was just my hand-wavy assumption that they must exist somewhere and will be executed here. I believe the primary concern about this change is that it could break podman (but we don't believe it will because podman is using a different conmon binary).

I did notice that there appear to be some podman tests in the Fedora repo here: https://src.fedoraproject.org/rpms/conmon/blob/rawhide/f/tests. I don't know what they do, though.

Ack. I guess we can leave TMT enablement and cirrus removal for another PR. This PR should be good to go.

@mdbooth mdbooth mentioned this pull request Sep 2, 2024
Open
saschagrunert
saschagrunert approved these changes Sep 3, 2024
View reviewed changes
Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

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

@haircommander PTAL

@haircommander haircommander merged commit 46c57f2 into containers:main Sep 5, 2024
28 of 29 checks passed
@mdbooth
Copy link

mdbooth commented Sep 5, 2024

Awesome, thanks! @lsm5 Any idea which version of Fedora this is likely to land in?

@lsm5 lsm5 deleted the remove-crio branch September 6, 2024 10:01
@lsm5
Copy link
Member Author

lsm5 commented Sep 6, 2024

Awesome, thanks! @lsm5 Any idea which version of Fedora this is likely to land in?

@saschagrunert @haircommander is it worth cutting a new conmon for this? If not, I can add it manually.

@mdbooth
Copy link

mdbooth commented Sep 6, 2024

Awesome, thanks! @lsm5 Any idea which version of Fedora this is likely to land in?

@saschagrunert @haircommander is it worth cutting a new conmon for this? If not, I can add it manually.

To reiterate my use case: I cannot switch to the k8s cri-o packages until I update my base OS (Fedora CoreOS) to a release including this change.

@lsm5
Copy link
Member Author

lsm5 commented Sep 6, 2024

To reiterate my use case: I cannot switch to the k8s cri-o packages until I update my base OS (Fedora CoreOS) to a release including this change.

@mdbooth updates created https://bodhi.fedoraproject.org/updates/?search=conmon .

@haircommander
Copy link
Collaborator

yeah we could cut a new version, I also see you've patched the RPM which I'm fine with

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdbooth mdbooth mdbooth left review comments

@saschagrunert saschagrunert saschagrunert approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RPM installs to /usr/libexec/crio/conmon which should be owned by CRI-O
4 participants
@lsm5 @saschagrunert @mdbooth @haircommander