rpm: update config files for drop ins#795
Conversation
|
Packit jobs failed. @containers/packit-build please check. |
9694392 to
b390368
Compare
lsm5
left a comment
There was a problem hiding this comment.
To retain the files only if they have been modified, we'll need this scriptlet before the %files section and also keep the 2 %dir lines.
%posttrans
# Restore user-modified config files from .rpmsave
for file in \
%{_sysconfdir}/containers/policy.json \
%{_sysconfdir}/containers/registries.conf \
%{_sysconfdir}/containers/registries.conf.d/000-shortnames.conf \
%{_sysconfdir}/containers/registries.d/default.yaml \
%{_sysconfdir}/containers/registries.d/registry.redhat.io.yaml \
%{_sysconfdir}/containers/registries.d/registry.access.redhat.com.yaml
do
if [ -f "${file}.rpmsave" ]; then
mv "${file}.rpmsave" "${file}"
fi
done
We can test this once there are packit scratch builds.
@jnovy PTAL as well.
| %dir %{_sysconfdir}/containers/registries.conf.d | ||
| %dir %{_sysconfdir}/containers/registries.d |
There was a problem hiding this comment.
If we want user-modified files to remain after rpm upgrade, we should keep these dirs.
There was a problem hiding this comment.
ok I dropped them because they were no longer created so rpm failed. I guess I just need to keep creating them in the install section?
There was a problem hiding this comment.
I think so. Another untested option would be to list the dirs as %ghost %dir %{_sysconfdir}..., so rpm would ignore those dirs if they exist, but not sure if that'd affect the %posttrans. Could leave that for followups / if we notice any additional issues after this goes in.
|
@lsm5 Thank you, this did the trick when I tested on a local VM. The unmodified policy.json is gone, while the modified registries.conf is kept. RPM reports
And then the posttrans moved it back successfully. I guess this means the theoretically there is a small window where podman might get run and the registries.conf.rpmsave is not yet moved back so it runs without the proper config. Given the non atomic nature that is unavoidable and I doubt it would be encountered by anyone. |
|
For the record it would likely be good to have #753 merged and vendored into podman first as otherwise the code will not yet be ready to read the files there which could cause troubles on podman-next so I moving this back to draft but otherwise this should be good to go pending review. |
mtrmac
left a comment
There was a problem hiding this comment.
ACK. I’ll defer to up-to-date RPM experts on everything.
Ship the fedora overwrites as proper drop ins. This allows us to remove the fragile script to patch the files but also makes it much clearer that we can ship the real upstream default as main file and then only have small drop in files for the distro customization. Also move remaining files from /etc to /usr as the code should now search there and this better to not conflict with admin level overwrites. Signed-off-by: Paul Holzinger <pholzing@redhat.com>
mtrmac
left a comment
There was a problem hiding this comment.
Also, we have talked about preventing the use of the updated c-common with old Podman/Buildah/Skopeo.
If that will happen, where should that happen? I think Conflicts: … < … in this package is a natural place.
Yes I think that is fair, have we decided on what the next skopeo/buildah versions are? Looking at podman-next repo If at least the buildah and skopeo versions there are wrong (older than even the latest release) So I assume if I would add a conflict right now here that would break podman-next packages? would this be correct? |
|
looking at the current spec file it included the epoch, is that required for Conflicts to work? the podman-next repo contains a much higher epoch so would that always be newer than? |
We should include the Epoch as well and have it conflict on the |
In order to avoid someone installing the new config files that the old tools cannot read. Signed-off-by: Paul Holzinger <pholzing@redhat.com>
The new network code uses a different json format that only netavark v2 can understand so ensure we require that. Signed-off-by: Paul Holzinger <pholzing@redhat.com>
| Conflicts: podman < 5:6 | ||
| Conflicts: buildah < 2:1.44 | ||
| Conflicts: skopeo < 1:1.23 |
There was a problem hiding this comment.
This works.
PTAL as well @inknos @jankaluza @jnovy .
| # Default search registries for RHEL | ||
| unqualified-search-registries = ["registry.access.redhat.com", "registry.redhat.io", "docker.io"] | ||
|
|
||
| # Enforcing mode for short names is default for Fedora 34 and newer |
There was a problem hiding this comment.
Is this still relevant as this is RHEL registries?
There was a problem hiding this comment.
I think I should just drop the comment I guess
| @@ -179,6 +202,21 @@ ln -s ../../../..%{_sysconfdir}/yum.repos.d/redhat.repo %{buildroot}%{_datadir}/ | |||
| %{_datadir}/containers/containers.conf | |||
| %{_datadir}/containers/mounts.conf | |||
There was a problem hiding this comment.
off-topic: would it be helpful to add drop-in support for mounts? base mounts.conf currently has /usr/share/rhel/secrets:/run/secrets, which, even though it's a no-op for non-rhel distributions (iiuc), fits better as a drop-in, much in line with what this PR implements.
There was a problem hiding this comment.
that was not in scope for our config file rewrite. I have not seen user complains about it so I never considered it.
If you want to file an issue for it sure, but it won't be in time for 6.0. Adding drop ins later for this should be safe enough I guess as it would not be a breaking change for mounts.conf.
mtrmac
left a comment
There was a problem hiding this comment.
ACK.
(Marking as approved to unblock merging, but I’ll fully defer to experts on reviews / timing.)
Ship the fedora overwrites as proper drop ins. This allows us to remove the fragile script to patch the files but also makes it much clearer that we can ship the real upstream default as main file and then only have small drop in files for the distro customization.