-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
rpm: assure we build and use a rpm repository #4796
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ssbarnea The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
just an FYI: I think some of the changes here could conflict badly with #4762 |
@NevilleC Yep, there is some clashing "opportunity" around thse but I think we will manage it, probably by splitting out changes in smaller bits, followed by some rebases. My focus is to fix the package building which currently produces bad rpms. As soon we have this right it will be much easier to introduce the other changes, like making the build process more idempotent. I am really glad that you are working on this because I was a bit surprised to discover the lack of idempotency on the build part. |
@ssbarnea Please sign your PRs |
recheck-rdo |
Assures we also build a rpm repository and install rpms from inside it. This improves testing of packaging by assuring that we test the same way it is supposed to be used in production. Depends-On: https://review.rdoproject.org/r/24376 Signed-off-by: Sorin Sbarnea <[email protected]>
@NevilleC If you can take a look at the PR it woudl breat. It was one major bug now which I was not yet able to figure-out. Apparently Disabling system repo is not an option because it contains other dependencies needed by podman. We need to assure it installs the right version, maybe we should even double check and verify the version after install to avoid a false-positive build. |
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 know only enough about rpm building to be dangerous 😉 At initial glance, the work looks good and I appreciate the comments.
I was a bit surprised to discover the lack of idempotency on the build part.
Idea for future work: Cirrus-CI has a built-in handy caching mechanism. Perhaps the rpms could be built once per distro, cached, then re-used on their respective test VMs.
The same/similar technique could be used for debs as well (for re-use on the Ubuntu VMs).
Thirdly if we want, the test packages could conditionally be made available via the publishing mechanism we already use for zip and msi binaries.
package-install: package ## Install rpm packages via a local podman.repo | ||
# We use the repository approach with max priority and without disabling | ||
# system packages in order to detect if our packages are | ||
# creating any conflicts. |
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.
Suggestion: these comments will also display on stdout when the package-install
recipe is executing. Not a huge problem but does add a bit to output clutter. Would the comment be just as useful for maintainers if placed above the target?
☔ The latest upstream changes (presumably #4762) made this pull request unmergeable. Please resolve the merge conflicts. |
@ssbarnea: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
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 did a quick review and looks overall good to me.
Sorry for the conflict generated here. Let me know if I can help with this.
# --best needed to avoid accident where old rpms ones are picked instead | ||
sudo ${PKG_MANAGER} -y install --best --allowerasing --nogpgcheck podman podman-remote | ||
podman version | ||
# info may require sudo and will also verify conman compatibility |
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.
s/conman/conmon
@@ -47,7 +51,7 @@ else | |||
fi | |||
|
|||
echo ${PKGS[*]} | |||
sudo $pkg_manager install -y ${PKGS[*]} | |||
sudo $pkg_manager install --disablerepo podman -y ${PKGS[*]} |
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 a comment to explain why we need the --disablerepo podman
?
I will take care all comments tomorrow (and rebase). Hopefully I will find a way to solve the repo issue, which is still a mistery to me. At least on rhel-8.1 yum reports zero rpms in the newly created repo, even if the repodata files clearly indicate the two rpms that we build. Installing the rpms manually without a repository works well but I would prefer a full repo test. |
One problem is Epoch: In the fedpkg spec, the Epoch is set to 2. In fake-spec and in rhpkg it is unset. You can possibly get this to work in fedora-land by copying the fedpkg epoch directive:
...but then you run into:
As I seldom get tired of mentioning, it is a really really really bad idea to have a duplicate, unmaintained, inconsistent specfile in this repo. (Not your fault! Just a long-existing maintenance nightmare) |
Closing for the moment until I get the other changes in. |
@edsantiago @NevilleC @cevich Please check #4815 as I started a new PR to avoid the noise from this one. |
Assures we also build a rpm repository and install rpms from inside it.
This improves testing of packaging by assuring that we test the same
way it is supposed to be used in production.