CONSOLE-5293: Use console-downloads image instead of cli-artifacts - #1158
CONSOLE-5293: Use console-downloads image instead of cli-artifacts#1158jhadvig wants to merge 2 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@jhadvig: This pull request references CONSOLE-5293 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe pull request migrates the OpenShift console downloads service from a Python-based HTTP server (cli-artifacts image) to a compiled ChangesDownloads Service Migration
🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bindata/assets/deployments/Untitled`:
- Line 1: Delete the stray asset file named "Untitled" that contains only the
token "readOnlyRootFilesystem" from the assets/deployments bundle; remove it
from the repository and any asset-generation lists so it won't be packaged into
bindata, and if the readOnlyRootFilesystem setting was intended for a real
deployment manifest, move that key into the proper deployment YAML (not the
"Untitled" stub).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 07e1b599-077b-438b-adb5-324ed8e45c96
📒 Files selected for processing (4)
bindata/assets/deployments/Untitledbindata/assets/deployments/downloads-deployment.yamlmanifests/07-operator-ibm-cloud-managed.yamlmanifests/07-operator.yaml
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
manifests/**/*.yaml
⚙️ CodeRabbit configuration file
manifests/**/*.yaml: Review CVO manifests carefully.
These are deployed to clusters and changes have wide impact.
Files:
manifests/07-operator-ibm-cloud-managed.yamlmanifests/07-operator.yaml
bindata/assets/**/*.yaml
⚙️ CodeRabbit configuration file
bindata/assets/**/*.yaml: Review YAML assets for Kubernetes resource correctness.
Ensure proper annotations for cluster profiles.
Files:
bindata/assets/deployments/downloads-deployment.yaml
🔀 Multi-repo context openshift/console
[::openshift/console::] README.md — references the origin-cli-artifacts image and oc binaries from quay.io/repository/openshift/origin-cli-artifacts (match at README.md:443 and README.md:463).
[::openshift/console::] Dockerfile.downloads — builds FROM quay.io/openshift/origin-cli-artifacts:4.18 and labels image as "openshift,cli-artifacts" (matches at Dockerfile.downloads:1 and Dockerfile.downloads:36).
Note: manifests/ and bindata/ were not present in this checkout (shell reported "No such file or directory"), so I could not locate the exact deployment YAML files changed in the PR within this repository.
🔇 Additional comments (1)
bindata/assets/deployments/downloads-deployment.yaml (1)
51-54: LGTM!
| @@ -0,0 +1 @@ | |||
| readOnlyRootFilesystem No newline at end of file | |||
There was a problem hiding this comment.
Remove this stray file.
bindata/assets/deployments/Untitled is not valid deployment YAML and looks like an accidental editor artifact. Leaving it under the assets tree risks confusing future maintainers or being swept into asset generation by mistake.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bindata/assets/deployments/Untitled` at line 1, Delete the stray asset file
named "Untitled" that contains only the token "readOnlyRootFilesystem" from the
assets/deployments bundle; remove it from the repository and any
asset-generation lists so it won't be packaged into bindata, and if the
readOnlyRootFilesystem setting was intended for a real deployment manifest, move
that key into the proper deployment YAML (not the "Untitled" stub).
There was a problem hiding this comment.
@TheRealJon, glad we're on the same page! Once the bindata/assets/deployments/Untitled file is removed from the PR, feel free to let me know and I'll mark this resolved.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bindata/assets/deployments/downloads-deployment.yaml`:
- Around line 51-54: The deployment currently runs the downloads binary
(command: /opt/downloads/downloads) with a readOnlyRootFilesystem but lacks a
writable /tmp, causing the Go server's os.MkdirTemp("", "artifacts") to fail;
update the Downloads Deployment to add a volumeMount for /tmp (mountPath: /tmp)
in the container spec and add a corresponding emptyDir volume (name like tmp or
downloads-tmp) in the pod spec so the container has a writable /tmp when
readOnlyRootFilesystem is true.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 023cf83e-cb7f-47c2-b8e2-1271d4209157
⛔ Files ignored due to path filters (1)
pkg/console/subresource/deployment/deployment_test.gois excluded by!**/*_test.go
📒 Files selected for processing (4)
bindata/assets/deployments/Untitledbindata/assets/deployments/downloads-deployment.yamlmanifests/07-operator-ibm-cloud-managed.yamlmanifests/07-operator.yaml
✅ Files skipped from review due to trivial changes (1)
- manifests/07-operator-ibm-cloud-managed.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- bindata/assets/deployments/Untitled
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
manifests/**/*.yaml
⚙️ CodeRabbit configuration file
manifests/**/*.yaml: Review CVO manifests carefully.
These are deployed to clusters and changes have wide impact.
Files:
manifests/07-operator.yaml
bindata/assets/**/*.yaml
⚙️ CodeRabbit configuration file
bindata/assets/**/*.yaml: Review YAML assets for Kubernetes resource correctness.
Ensure proper annotations for cluster profiles.
Files:
bindata/assets/deployments/downloads-deployment.yaml
🔀 Multi-repo context openshift/console
[::openshift/console::] Dockerfile.downloads:1 — FROM quay.io/openshift/origin-cli-artifacts:4.18 (file shows image built-from still references origin-cli-artifacts).
[::openshift/console::] Dockerfile.downloads:36 — image label io.openshift.tags="openshift,cli-artifacts".
[::openshift/console::] README.md:443,463 — README documents that the server provides oc binaries from quay.io/repository/openshift/origin-cli-artifacts and notes origin-cli-artifacts usage for local builds.
[::openshift/console::] Dockerfile.downloads:20-21,28 — the repo already builds and bundles the Go downloads server binary (/opt/downloads/downloads) and defaultArtifactsConfig.yaml; Dockerfile.downloads CMD is "/opt/downloads/downloads --config-path=/opt/downloads/defaultArtifactsConfig.yaml".
[::openshift/console::] cmd/downloads/main.go & cmd/downloads/config/* — implementation of the Go downloads server and config generation present in-tree (server listens on configured port and serves the generated TempDir).
Summary: repository contains the Go downloads server and Dockerfile that builds an image from origin-cli-artifacts base; README also references origin-cli-artifacts. The PR switching operator manifests to use registry.svc.ci.openshift.org/openshift:console-downloads will require the console-downloads image to be produced/pushed (openshift/release) and reconciled with the existing Dockerfile.downloads / README references that still mention origin-cli-artifacts.
🔇 Additional comments (1)
manifests/07-operator.yaml (1)
72-73: LGTM!
|
/retest |
TheRealJon
left a comment
TheRealJon
left a comment
There was a problem hiding this comment.
Just the stray file to remove, other than that it looks good!
| @@ -0,0 +1 @@ | |||
| readOnlyRootFilesystem No newline at end of file | |||
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhadvig, TheRealJon 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 |
|
/retest |
|
New changes are detected. LGTM label has been removed. |
Replace the cli-artifacts image with the new console-downloads image for the downloads deployment. The new image contains a Golang HTTP server with its own entrypoint, so the inline Python server script and associated command/args/volumes are removed from the deployment template. The Go downloads server writes to /tmp/artifacts at startup (readOnlyRootFilesystem is true), so a writable tmpfs is required.
|
/retest |
1 similar comment
|
/retest |
|
/retest |
|
@jhadvig: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Analysis / Root cause:
The downloads deployment currently uses the
cli-artifactsimage and runs an inline Python HTTP server script to serve CLI binaries. This approach is being replaced by a dedicatedconsole-downloadsimage(openshift/console Dockerfile.downloads) that contains a purpose-built Golang HTTP server with its own entrypoint, making the inline Python
script unnecessary.
Jira: https://redhat.atlassian.net/browse/CONSOLE-5293
Solution description:
manifests/07-operator.yamlandmanifests/07-operator-ibm-cloud-managed.yaml— Updated theDOWNLOADS_IMAGEenv var fromregistry.svc.ci.openshift.org/openshift:cli-artifactstoregistry.svc.ci.openshift.org/openshift:console-downloads.bindata/assets/deployments/downloads-deployment.yaml— Removed the inline Python HTTP server script (command,args), the/tmpvolumeMount, and thetmpemptyDirvolume. The newconsole-downloadsimage has its own entrypoint (/opt/downloads/downloads --config-path=/opt/downloads/defaultArtifactsConfig.yaml), so no custom command/args are needed.No Go code changes were required — the operator reads the deployment YAML from bindata and sets the image from the
DOWNLOADS_IMAGEenv var, which continues to work as before.Test setup:
The
console-downloadsimage must be present in the release payload. Ensure the image is built and available via the CI config in openshift/release.Test cases:
console-downloadsimagehttps://downloads-openshift-console.apps.<cluster>/serves CLI binaries correctlyocdownload links for all architectures (amd64, arm64, ppc64le, s390x) and platforms (linux, mac, windows) are functionalcli-artifactsNeeds to be tested with openshift/console#16455
Depends on: openshift/release#79355
Browser conformance:
Additional info:
This is a net deletion of ~200 lines of inline Python. The new Golang-based downloads server in the
console-downloadsimage provides the same functionality with better performance and maintainability.Summary by CodeRabbit
download-serverbinary directly with the default artifacts configuration.console-downloadsimage for more consistent runtime assets.