fix: revert oc download from cluster#364
Conversation
Signed-off-by: lugi0 <lgiorgi@redhat.com>
📝 WalkthroughSummary by CodeRabbit
Summary by CodeRabbit
WalkthroughThis change updates the Dockerfile to install the Rosa CLI and OpenShift CLI (oc) statically during image build. It also comments out Python code and fixtures in the test and utility modules that previously handled dynamic downloading and environment setup for the oc CLI, removing related imports as well. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
/build-push-pr-image |
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/cherry-pick', '/build-push-pr-image', '/hold', '/verified', '/wip', '/lgtm'} |
|
Status of building tag pr-364: success. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Dockerfile (1)
20-25: Add architecture specification for consistency and consider version pinning.The Rosa CLI download uses a generic
rosa-linux.tar.gzwhile the OC CLI download specifiesx86_64architecture. For consistency and to avoid potential architecture mismatches, consider specifying the architecture for Rosa CLI as well.Additionally, using "latest" for Rosa CLI could lead to non-reproducible builds.
# Install the Rosa CLI -RUN curl -L https://mirror.openshift.com/pub/openshift-v4/clients/rosa/latest/rosa-linux.tar.gz --output /tmp/rosa-linux.tar.gz \ +RUN curl -L https://mirror.openshift.com/pub/openshift-v4/clients/rosa/latest/rosa-linux-amd64.tar.gz --output /tmp/rosa-linux.tar.gz \ && tar xvf /tmp/rosa-linux.tar.gz --no-same-owner \ && mv rosa /usr/bin/rosa \ && chmod +x /usr/bin/rosa \ + && rm -f /tmp/rosa-linux.tar.gz \ && rosa version
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Dockerfile(1 hunks)tests/conftest.py(1 hunks)utilities/infra.py(1 hunks)
🔇 Additional comments (2)
utilities/infra.py (1)
1149-1231: Verify that commented functions are not referenced elsewhere in the codebase.The OC CLI download functions have been properly commented out, which aligns with the shift to Docker-based CLI installation. The commenting approach (rather than deletion) allows for potential rollback if needed.
Please verify that no other parts of the codebase still reference these commented functions:
#!/bin/bash # Description: Search for any remaining references to the commented OC CLI functions # Expected: No active references should be found echo "Searching for references to commented functions..." # Search for function calls rg -A 3 "get_machine_platform|get_os_system|get_oc_console_cli_download_link|get_server_cert|download_oc_console_cli" --type py echo -e "\nSearching for imports of commented functions..." rg "from.*infra.*import.*download_oc_console_cli" --type pytests/conftest.py (1)
529-563: Verify that no tests depend on the commented fixtures.The pytest fixtures for OC CLI binary management have been properly commented out, which is consistent with the shift to Docker-based CLI installation. The
autouse_fixturesfixture that controlled execution order is also appropriately disabled.Please verify that no test files still reference these commented fixtures:
#!/bin/bash # Description: Search for any remaining references to the commented fixtures # Expected: No active fixture references should be found echo "Searching for references to commented fixtures..." # Search for fixture usage rg -A 2 "os_path_environment|bin_directory|bin_directory_to_os_path|oc_binary_path" --type py tests/ echo -e "\nSearching for autouse_fixtures references..." rg "autouse_fixtures" --type py tests/
| # Install the OpenShift CLI (OC) | ||
| RUN curl -L https://mirror.openshift.com/pub/openshift-v4/x86_64/clients/ocp/stable/openshift-client-linux.tar.gz --output /tmp/openshift-client-linux.tar.gz \ | ||
| && tar xvf /tmp/openshift-client-linux.tar.gz --no-same-owner \ | ||
| && mv oc /usr/bin/oc \ | ||
| && chmod +x /usr/bin/oc |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add verification command for OC CLI installation.
The Rosa CLI installation includes a verification step (rosa version), but the OC CLI installation lacks similar verification. This could lead to silent failures during the build process.
# Install the OpenShift CLI (OC)
RUN curl -L https://mirror.openshift.com/pub/openshift-v4/x86_64/clients/ocp/stable/openshift-client-linux.tar.gz --output /tmp/openshift-client-linux.tar.gz \
&& tar xvf /tmp/openshift-client-linux.tar.gz --no-same-owner \
&& mv oc /usr/bin/oc \
- && chmod +x /usr/bin/oc
+ && chmod +x /usr/bin/oc \
+ && rm -f /tmp/openshift-client-linux.tar.gz \
+ && oc version --client📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Install the OpenShift CLI (OC) | |
| RUN curl -L https://mirror.openshift.com/pub/openshift-v4/x86_64/clients/ocp/stable/openshift-client-linux.tar.gz --output /tmp/openshift-client-linux.tar.gz \ | |
| && tar xvf /tmp/openshift-client-linux.tar.gz --no-same-owner \ | |
| && mv oc /usr/bin/oc \ | |
| && chmod +x /usr/bin/oc | |
| # Install the OpenShift CLI (OC) | |
| RUN curl -L https://mirror.openshift.com/pub/openshift-v4/x86_64/clients/ocp/stable/openshift-client-linux.tar.gz --output /tmp/openshift-client-linux.tar.gz \ | |
| && tar xvf /tmp/openshift-client-linux.tar.gz --no-same-owner \ | |
| && mv oc /usr/bin/oc \ | |
| && chmod +x /usr/bin/oc \ | |
| && rm -f /tmp/openshift-client-linux.tar.gz \ | |
| && oc version --client |
🤖 Prompt for AI Agents
In Dockerfile lines 27 to 31, the OpenShift CLI (OC) installation lacks a
verification step to confirm successful installation. Add a command after
setting permissions to run `oc version` or a similar command to verify the OC
CLI is correctly installed and executable, ensuring any installation issues are
caught during the build process.
|
/lgtm |
| os.chmod(binary_path, stat.S_IRUSR | stat.S_IXUSR) | ||
| return binary_path | ||
| # def get_machine_platform() -> str: | ||
| # os_machine_type = platform.machine() |
There was a problem hiding this comment.
Wondering if commenting is the best way to stow away changes, I will not block it though.
There was a problem hiding this comment.
Tox seems to be complaining about unused code https://github.com/opendatahub-io/opendatahub-tests/actions/runs/15758251029/job/44418477232?pr=364
I might be wrong but if true, maybe deleting this and reverting it later is the better option, else it will complain everytime Tox runs.
dbasunag
left a comment
dbasunag
left a comment
There was a problem hiding this comment.
Please don't merge the revert.
|
#366 fixes this issue. |
|
Closing this as the issue was addressed in #366 |
7 participants
Description
How Has This Been Tested?
Merge criteria: