libvirt CI Dockerfile fix #1414
Conversation
openshift-ci-robot
added
the
size/XS
|
/assign @praveenkumar |
|
@abhinavdahiya: GitHub didn't allow me to assign the following users: praveenkumar. Note that only openshift members and repo collaborators can be assigned and that issues/PRs can only have 10 assignees at the same time. DetailsIn response to this:
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. |
|
/cc @praveenkumar |
|
@abhinavdahiya: GitHub didn't allow me to request PR reviews from the following users: praveenkumar. Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs. DetailsIn response to this:
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. |
images/nested-libvirt/Dockerfile
Outdated
There was a problem hiding this comment.
Might as well update it to dnf. :)
There was a problem hiding this comment.
No yum is now symlink for fedora so it will work across platform (be it fedora/centos or RHEL).
There was a problem hiding this comment.
@sallyom so this change is done because this package is coming from the epel repo instead the epel-testing now. I am hoping initially we did this because specific version we wanted for nss-wrapper not made to the stable repo.
$ yum info nss_wrapper
Failed to set locale, defaulting to C
Loaded plugins: fastestmirror
Loading mirror speeds from cached hostfile
* base: centos4.zswap.net
* epel: mirror.cogentco.com
* extras: centos4.zswap.net
* updates: centos4.zswap.net
Available Packages
Name : nss_wrapper
Arch : x86_64
Version : 1.1.5
Release : 1.el7
Size : 37 k
Repo : epel/x86_64
Summary : A wrapper for the user, group and hosts NSS API
URL : https://cwrap.org/
License : BSD
...
There was a problem hiding this comment.
I am hoping initially we did this because specific version we wanted for nss-wrapper not made to the stable repo.
I went back through #625 and didn't see any discussion of --enablerepo=epel-testing, so I'm fine dropping it here. We can always restore it later with a commit message about why it is useful, if it turns out that we do, in fact, need it.
|
LGTM. One nitpick though: It would be super awesome if we also add details mentioned in the description here to git commit message. From personal experience, it's a blessing when you can tell the rational for changes w/o having search on github. |
images/nested-libvirt/Dockerfile
Outdated
There was a problem hiding this comment.
@sallyom so this change is done because this package is coming from the epel repo instead the epel-testing now. I am hoping initially we did this because specific version we wanted for nss-wrapper not made to the stable repo.
$ yum info nss_wrapper
Failed to set locale, defaulting to C
Loaded plugins: fastestmirror
Loading mirror speeds from cached hostfile
* base: centos4.zswap.net
* epel: mirror.cogentco.com
* extras: centos4.zswap.net
* updates: centos4.zswap.net
Available Packages
Name : nss_wrapper
Arch : x86_64
Version : 1.1.5
Release : 1.el7
Size : 37 k
Repo : epel/x86_64
Summary : A wrapper for the user, group and hosts NSS API
URL : https://cwrap.org/
License : BSD
...
|
/lgtm Do update the commit for future reference. Thanks for putting it together. |
|
@praveenkumar: changing LGTM is restricted to assignees, and only openshift/installer repo collaborators may be assigned issues. DetailsIn response to this:
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. |
|
/lgtm |
sallyom
force-pushed
the
fixup-libvirt-ci-dockerfile
branch
from
ccbd4cf to
5a310b3
Compare
|
@wking, I updated the commit msg, thanks |
sallyom
force-pushed
the
fixup-libvirt-ci-dockerfile
branch
from
5a310b3 to
9e65a94
Compare
|
/lgtm |
openshift-ci-robot
added
lgtm
|
/lgtm cancel |
openshift-ci-robot
removed
lgtm
|
/lgtm (sorry for the flip-flops, I was momentarily confused by the |
openshift-ci-robot
added
lgtm
openshift-ci-robot
removed
the
approved
sallyom
force-pushed
the
fixup-libvirt-ci-dockerfile
branch
from
9e65a94 to
a754aa8
Compare
Originally we installed nss_wrapper package from epel-testing, I think because it wasn't available in epel repo (I'm not 100%sure) We can now install from the stable epel repo, so no longer need the epel-testing repo. That's good, because epel-testing is no longer configured in the base image (the build was failing until I removed it, and I realized we no longer needed it).
sallyom
force-pushed
the
fixup-libvirt-ci-dockerfile
branch
from
a754aa8 to
a9f4b4e
Compare
|
/lgtm |
openshift-ci-robot
added
lgtm
|
/lgtm cancel Heh, I just can't make up my mind on this one :p. I think we still need to address this by keeping a separate |
openshift-ci-robot
removed
lgtm
|
@wking nope you were right with the |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cgwalters, sallyom, wking The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
|
/test e2e-aws |
8 participants
This PR updates the Dockerfile used in CI to build the libvirt image. Repo epel-testing is not available, so build errors out. We don't need this so I've removed it.
I'm working to get the libvirt CI job running, with CRC team.