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.

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

[skip-ci] RPM: ensure config files are patched #2026

Merged
merged 2 commits into from
May 30, 2024

Conversation

lsm5
Copy link
Member

@lsm5 lsm5 commented May 30, 2024

This commit copies over shortnames.conf, registries.conf and storage.conf to the rpm build dir before running the patching script.

Prior to this, the script would complain about not finding those files but the rpmbuild process itself didn't fail.

@lsm5
Copy link
Member Author

lsm5 commented May 30, 2024

@edsantiago @Luap99 @jnovy PTAL

@lsm5
Copy link
Member Author

lsm5 commented May 30, 2024

@jnovy @inknos do you know how to ensure rpmbuild fails if a script used in the setup or build stages fails to find a file? I thought -e in the script should've been enough (?)

@Luap99
Copy link
Member

Luap99 commented May 30, 2024

@jnovy @inknos do you know how to ensure rpmbuild fails if a script used in the setup or build stages fails to find a file? I thought -e in the script should've been enough (?)

The script exits 0 when I execute in the repo root here despite not having all the files, the reason is that the code does if grep ... so a missing file is the same as no match and the condition just goes into the else so there is never a error thrown by the script.

@lsm5
Copy link
Member Author

lsm5 commented May 30, 2024

moved zstd:chunked to rawhide and rhel > 10.

@rhatdan
Copy link
Member

rhatdan commented May 30, 2024

LGTM

lsm5 added 2 commits May 30, 2024 10:15
This commit copies over shortnames.conf, registries.conf
and storage.conf to the rpm build dir before running the patching
script.

Prior to this, the script would complain about not finding those files
but the rpmbuild process itself didn't fail.

Signed-off-by: Lokesh Mandvekar <lsm5@redhat.com>
Signed-off-by: Lokesh Mandvekar <lsm5@redhat.com>
Copy link
Contributor

openshift-ci bot commented May 30, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago, lsm5

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jnovy
Copy link
Contributor

jnovy commented May 30, 2024

@Luap99 In general RPM scriptlets should never fail as a it would break the whole transaction leaving the system in inconsistent state - it can put a message in the log though.

@Luap99
Copy link
Member

Luap99 commented May 30, 2024

@Luap99 In general RPM scriptlets should never fail as a it would break the whole transaction leaving the system in inconsistent state - it can put a message in the log though.

But we are not talking about a rpm scriptlet are we? This here patches the config files at rpm build time so we very much want to know if if failed to patch the file otherwise we install invalid config files

@lsm5
Copy link
Member Author

lsm5 commented May 30, 2024

let's make sure to get this one in before the new c/common cut tomorrow.

@edsantiago
Copy link
Member

/lgtm

This is specfile, not scriptlet. Failing in rpmbuild is the right thing to do.

@openshift-ci openshift-ci bot added the lgtm label May 30, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 18fdea5 into containers:main May 30, 2024
12 checks passed
@lsm5
Copy link
Member Author

lsm5 commented May 30, 2024

/cherrypick v0.59

@jnovy
Copy link
Contributor

jnovy commented May 31, 2024

LGTM

@travier
Copy link
Member

travier commented Jun 11, 2024

d3ca62d (#2026) is missing from 0.59 and is causing issues:

See: 5c5b112#r143013047

@cgwalters

This comment was marked as outdated.

@cgwalters
Copy link
Contributor

➡️ #2048

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants