Combine ubuntu and centos build scripts#13
Conversation
smoshiur1237
left a comment
smoshiur1237
left a comment
There was a problem hiding this comment.
Small nit
otherwise LGTM
nuhakala
force-pushed
the
nuhakala/combine-build-scripts
branch
2 times, most recently
from
b9cac3c to
9cff8c7
Compare
nuhakala
force-pushed
the
nuhakala/combine-build-scripts
branch
from
9cff8c7 to
9666da7
Compare
| "sles-ipa-install" "${IPA_BASE_OS}" "sles-zypper-config" "sles-ipa-ramdisk-base" \ | ||
| "dynamic-login" "journal-to-console" "devuser" "openssh-server" "sles-extra-hardware" \ | ||
| "ipa-module-autoload" "simple-init" "override-simple-init" -o "${IPA_IMAGE_NAME}" | ||
| "ipa-module-autoload" "${SIMPLE_INIT_ELEMENTS}" -o "${IPA_IMAGE_NAME}" |
There was a problem hiding this comment.
Have you run this with this "SIMPLE_INIT_ENABLED"?
I had issues in the past extending argument list like this. I would think that disk-image-create would try to handle the subtitution like this "simple-init override-simple-init" as a single argument.
There was a problem hiding this comment.
I have, it expands correctly on ubuntu and centos.
There was a problem hiding this comment.
I don't know here, but generally array and [@] would be a safer choice.
nuhakala
force-pushed
the
nuhakala/combine-build-scripts
branch
from
9666da7 to
d8d201c
Compare
There was a problem hiding this comment.
Pull request overview
This PR consolidates the Ubuntu and CentOS build scripts into a single unified script that supports both operating systems. The changes include removing deprecated glean/simple-init configurations, adding OS detection logic, and updating package installation commands for CentOS Stream 9 compatibility.
Key Changes:
- Unified build script with OS detection supporting both Ubuntu and CentOS
- Migrated from
virtualenvcommand topython3 -m venvfor virtual environment creation - Made simple-init elements optional via new
INCLUDE_SIMPLE_INITenvironment variable
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
build_ipa_sles_on_ubuntu.sh |
Removed - functionality merged into unified script |
build_ipa_sles.sh |
Enhanced with OS detection, conditional package installation, and optional simple-init support |
testenv.sh |
Added INCLUDE_SIMPLE_INIT environment variable defaulting to false |
README.md |
Documented the new INCLUDE_SIMPLE_INIT configuration option |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Build the IPA initramfs and kernel images | ||
| ################################################# | ||
| SIMPLE_INIT_ELEMENTS="" | ||
| if [[ "${INCLUDE_SIMPLE_INIT}" == true ]]; then |
There was a problem hiding this comment.
The string comparison "${INCLUDE_SIMPLE_INIT}" == true compares against the literal string "true", not a boolean. While this works, it's inconsistent with the usage pattern elsewhere in the script where boolean variables are tested directly (e.g., if $ENABLE_BOOTSTRAP_TEST on line 138).
For consistency with the codebase, consider either:
- Using
if [[ "${INCLUDE_SIMPLE_INIT}" == "true" ]](with quotes around "true"), or - Testing directly:
if $INCLUDE_SIMPLE_INIT(matching line 138's pattern)
Note: If using option 2, ensure the variable is set to true or false without quotes.
| if [[ "${INCLUDE_SIMPLE_INIT}" == true ]]; then | |
| INCLUDE_SIMPLE_INIT=${INCLUDE_SIMPLE_INIT:-false} | |
| if $INCLUDE_SIMPLE_INIT; then |
There was a problem hiding this comment.
Keep string test, the binary true/false is very confusing these days.
There was a problem hiding this comment.
Added quotes around "true". I think it does not really matter, but at least it is the same as other if-clauses in the script.
Signed-off-by: Nuutti Hakala <nuutti.hakala@est.tech>
nuhakala
force-pushed
the
nuhakala/combine-build-scripts
branch
from
d8d201c to
8eeb16e
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Rozzii, terror96 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 |
This PR combines the two existing build scripts. Also, the build script is updated for CentOS Stream 9: the package names have slightly changed and also creating python venv needed to adjust.