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

move required build files to configmap for clarity #4847

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cheesesashimi
Copy link
Member

@cheesesashimi cheesesashimi commented Feb 11, 2025

- What I did

The Containerfile template has a lot of indirection contained within it which
is very difficult to reason about and debug, especially when templating is
involved. To make this easier, I extracted as much as possible from the various
build scripts and pushed those scripts into the Containerfile ConfigMap via the
buildrequest/assets directory.

- How to verify it

Everything should continue to work as-is. This should be tested against a proxy-enabled cluster just to validate that these changes did not have any unintended side-effects.

- Description for the changelog
Move required build files to ConfigMap for clarity

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 11, 2025
Copy link
Contributor

openshift-ci bot commented Feb 11, 2025

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Copy link
Contributor

openshift-ci bot commented Feb 11, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cheesesashimi

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 11, 2025
@cheesesashimi cheesesashimi force-pushed the zzlotnik/fix-extensions-containerfile branch from 0630835 to 8976475 Compare February 11, 2025 22:07
@RishabhSaini
Copy link
Contributor

Having looked at the changes to the buildah-build.sh script and the inner-script, they both correctly utilize users.
LGTM

@RishabhSaini
Copy link
Contributor

I had a question of how install-extensions.sh was getting called and with what packages as arguments?

@cheesesashimi cheesesashimi force-pushed the zzlotnik/fix-extensions-containerfile branch from 8976475 to 769d0cb Compare February 12, 2025 15:08
@cheesesashimi
Copy link
Member Author

I had a question of how install-extensions.sh was getting called and with what packages as arguments?

Those are the resolved packaegs that we get from the various extensions that someone wishes to install. Previously, we were just passing them to rpm-ostree install. However, we have a need to perform some post-installation actions on a per-package basis. While it would be great to do it all in the Containerfile without needing additional files and scripts, maintaining that is complicated, to say the least.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants