Skip to content

Conversation

@henrywang
Copy link
Collaborator

@henrywang henrywang commented Dec 10, 2025

The patch has been added in OSCI. Those updates need to be added here so the patch can be removed from OSCI in next release.

@bootc-bot bootc-bot bot requested a review from ckyrouac December 10, 2025 00:59
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request updates tests to work with OSCI gating. The changes involve adding new RHEL base images to os-image-map.json and modifying a test script to copy repository files on RHEL systems. My review focuses on improving the robustness and maintainability of the test script changes. I've identified a potential bug with a hardcoded repository filename and suggested a simplification for parsing the os-release file.

Comment on lines +47 to +48
cp /etc/yum.repos.d/rhel.repo .
$repo_copy = "COPY rhel.repo /etc/yum.repos.d/"
Copy link
Contributor

Choose a reason for hiding this comment

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

high

The filename rhel.repo appears to be incorrect. On standard RHEL systems, the main repository file is typically named redhat.repo. Using rhel.repo would likely cause the cp command to fail and break the test on a standard RHEL environment.

If rhel.repo is intentionally used and is specific to the OSCI environment, it would be helpful to add a comment explaining this context.

For better robustness and portability, you might consider copying all .repo files instead of relying on a single hardcoded name. This would make the test less brittle to changes in repository configuration. For example:

if $os.ID == "rhel" {
    cp (glob /etc/yum.repos.d/*.repo) .
    $repo_copy = "COPY *.repo /etc/yum.repos.d/"
}
        cp /etc/yum.repos.d/redhat.repo .
        $repo_copy = "COPY redhat.repo /etc/yum.repos.d/"

Comment on lines +37 to +43
let os = open /usr/lib/os-release
| lines
| filter {|l| $l != "" and not ($l | str starts-with "#") }
| parse "{key}={value}"
| reduce {|it, acc|
$acc | upsert $it.key ($it.value | str trim -c '"')
}
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The manual parsing of /usr/lib/os-release can be simplified. Nushell's from dotenv command can parse this file format directly, making the code more concise and readable.

    let os = open /usr/lib/os-release | from dotenv

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant