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.

Sign up for GitHub

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

Jump to bottom

⚠️ Remove rpm install method #585

Merged
merged 1 commit into from
Nov 5, 2024
Merged

⚠️ Remove rpm install method #585

merged 1 commit into from
Nov 5, 2024

Conversation

elfosardo
Copy link
Member

Closes #583

@metal3-io-bot metal3-io-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 5, 2024
@elfosardo
Copy link
Member Author

/test metal3-centos-e2e-integration-test-main metal3-ubuntu-e2e-integration-test-main

@elfosardo
Remove rpm install method
050892e 
Closes metal3-io#583

Signed-off-by: Riccardo Pittau <[email protected]>
@elfosardo
Copy link
Member Author

/test metal3-centos-e2e-integration-test-main metal3-ubuntu-e2e-integration-test-main

tuminoid
tuminoid reviewed Nov 5, 2024
View reviewed changes
Copy link
Member

@tuminoid tuminoid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: consistent curlys + use array for BUILD_DEPS for cleaner code, while touching those lines.

prepare-image.sh
sed -i '/^sushy===/d' "$UPPER_CONSTRAINTS_PATH"
fi
if [[ -n ${SUSHY_SOURCE:-} ]]; then
sed -i '/^sushy===/d' "$UPPER_CONSTRAINTS_PATH"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
sed -i '/^sushy===/d' "$UPPER_CONSTRAINTS_PATH"
sed -i '/^sushy===/d' "${UPPER_CONSTRAINTS_PATH}"

prepare-image.sh
sed -i '/^ironic-lib===/d' "$UPPER_CONSTRAINTS_PATH"
fi
if [[ -n ${IRONIC_LIB_SOURCE:-} ]]; then
sed -i '/^ironic-lib===/d' "$UPPER_CONSTRAINTS_PATH"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
sed -i '/^ironic-lib===/d' "$UPPER_CONSTRAINTS_PATH"
sed -i '/^ironic-lib===/d' "${UPPER_CONSTRAINTS_PATH}"

prepare-image.sh

python3 -m pip install --no-cache-dir --ignore-installed --prefix /usr -r "$IRONIC_PKG_LIST_FINAL" -c "${UPPER_CONSTRAINTS_PATH}"
python3 -m pip install --no-cache-dir --ignore-installed --prefix /usr -r "$IRONIC_PKG_LIST_FINAL" -c "${UPPER_CONSTRAINTS_PATH}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
python3 -m pip install --no-cache-dir --ignore-installed --prefix /usr -r "$IRONIC_PKG_LIST_FINAL" -c "${UPPER_CONSTRAINTS_PATH}"
python3 -m pip install --no-cache-dir --ignore-installed --prefix /usr -r "${IRONIC_PKG_LIST_FINAL}" -c "${UPPER_CONSTRAINTS_PATH}"

prepare-image.sh
# emulate uid/gid configuration to match rpm install
IRONIC_UID=997
IRONIC_GID=994
BUILD_DEPS="python3-devel gcc git-core python3-setuptools python3-jinja2"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are touching these lines now, let's use modern syntax and avoid shellcheck ignores later on. Also makes cleaner PRs going forward. Also sorted.

Suggested change
BUILD_DEPS="python3-devel gcc git-core python3-setuptools python3-jinja2"
declare -a BUILD_DEPS=(
gcc
git-core
python3-devel
python3-jinja2
python3-setuptools
)

prepare-image.sh
dnf upgrade -y
# NOTE(dtantsur): pip is a requirement of python3 in CentOS
# shellcheck disable=SC2086
dnf install -y python3-pip $BUILD_DEPS
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
dnf install -y python3-pip $BUILD_DEPS
dnf install -y python3-pip "${BUILD_DEPS[@]}"

(only if using array when defining the list)

prepare-image.sh
fi
# clean installed build dependencies
# shellcheck disable=SC2086
dnf remove -y $BUILD_DEPS
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
dnf remove -y $BUILD_DEPS
dnf remove -y "${BUILD_DEPS[@]}"

(only if using array when defining the list)

prepare-image.sh
dnf remove -y $BUILD_DEPS
fi
# clean installed build dependencies
# shellcheck disable=SC2086
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# shellcheck disable=SC2086

No longer needed with new syntax

prepare-image.sh
BUILD_DEPS="python3-devel gcc git-core python3-setuptools python3-jinja2"
dnf upgrade -y
# NOTE(dtantsur): pip is a requirement of python3 in CentOS
# shellcheck disable=SC2086
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# shellcheck disable=SC2086

No longer needed with new syntax

@elfosardo
Copy link
Member Author

@tuminoid thanks for the comments and suggestions, I didn't want to charge the PR too much so I just went for the minimum required changes, if that's ok for you I would apply your suggestions in a follow-up since it's passing CI

@dtantsur
Copy link
Member

dtantsur commented Nov 5, 2024

/approve

I agree with the comments. But since they concern the already existing code, I think it's a good idea to address them separately.

@metal3-io-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dtantsur

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

@metal3-io-bot metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 5, 2024
@tuminoid
Copy link
Member

tuminoid commented Nov 5, 2024

Follow up is ok as well.

tuminoid
tuminoid reviewed Nov 5, 2024
View reviewed changes
Copy link
Member

@tuminoid tuminoid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 5, 2024
@metal3-io-bot metal3-io-bot merged commit debc9a7 into metal3-io:main Nov 5, 2024
7 checks passed
elfosardo added a commit to elfosardo/ironic-image that referenced this pull request Nov 5, 2024
@elfosardo
Improve bash syntax in prepare-image
66294b3 
Follow up to metal3-io#585
@elfosardo elfosardo mentioned this pull request Nov 5, 2024
Merged
elfosardo added a commit to elfosardo/ironic-image that referenced this pull request Nov 5, 2024
@elfosardo
Improve bash syntax in prepare-image
bc5071f 
Follow up to metal3-io#585

Signed-off-by: Riccardo Pittau <[email protected]>
iurygregory pushed a commit to iurygregory/ironic-image that referenced this pull request Nov 12, 2024
@elfosardo
Merge pull request metal3-io#585 from dtantsur/CVE-2024-44082-4.13
ae8747c 
OCPBUGS-39384,OCPBUGS-37764: Include fixes for CVE-2024-44082
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tuminoid tuminoid tuminoid left review comments

@dtantsur dtantsur Awaiting requested review from dtantsur

@iurygregory iurygregory Awaiting requested review from iurygregory

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

rpm installation is deprecated and should be removed
4 participants
@elfosardo @dtantsur @metal3-io-bot @tuminoid