AGENT-1261: Update dependency sourcing to remote#3152
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@pawanpinjarkar: This pull request references AGENT-1261 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 sub-task to target the "4.21.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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pawanpinjarkar The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughThe Containerfile.ocp is replaced with a Cachito-backed, offline multi-stage build: it copies remote artifacts, installs a pinned Yarn, builds UI assets offline, compiles a deterministic Go proxy binary from cached sources, and assembles both into the final runtime image serving on port 8080. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant CI as Build System
participant RS as Remote Sources (Cachito)
participant UI as ui-build
participant PB as proxy-build
participant Final as Final Image
Dev->>CI: docker build (Containerfile.ocp) with REMOTE_SOURCES
CI->>RS: copy/mount $REMOTE_SOURCES_DIR (Cachito artifacts)
RS-->>CI: yarn tarball, .npmrc/.yarnrc/lockfiles, gomod artifacts
CI->>UI: COPY remote sources -> build context
UI->>UI: install YARN from tarball
UI->>UI: use Cachito configs, `yarn install --immutable`
UI->>UI: `yarn build:all` (offline)
note right of UI: offline UI asset build
CI->>PB: COPY cachito-gomod-with-deps/app -> /app
PB->>PB: set CGO_ENABLED=0, vendor-aware build
PB->>PB: `go build -o assisted-disconnected-ui` (deterministic)
note right of PB: deterministic Go proxy build
CI->>Final: COPY ui-build:/app/proxy/dist -> /app/proxy/dist
CI->>Final: COPY proxy-build:/app/proxy/assisted-disconnected-ui -> /app/proxy/
Final-->>Dev: final image (UI assets + proxy binary) serving on :8080
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
openshift-ci
bot
added
the
needs-ok-to-test
|
Welcome @pawanpinjarkar! It looks like this is your first PR to openshift-assisted/assisted-installer-ui 🎉 |
|
Hi @pawanpinjarkar. Thanks for your PR. I'm waiting for a openshift-assisted member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
openshift-ci
bot
added
the
size/M
|
@pawanpinjarkar: This pull request references AGENT-1261 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 sub-task to target the "4.21.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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
apps/assisted-disconnected-ui/Containerfile.ocp.art (4)
14-22: Yarn bootstrap approach is brittle; prefer corepack or verify the tarball method.npm -g for Yarn v3 is non-standard; corepack is the recommended path. If offline-only, keep the tarball, else switch.
USER 0 ARG YARN_VERSION=v3.4.1 -RUN CACHED_YARN=./artifacts/yarn-${YARN_VERSION}.tar.gz; \ - if [ -f ${CACHED_YARN} ]; then \ - npm install -g ${CACHED_YARN}; \ - else \ - echo "need yarn at ${CACHED_YARN}"; \ - exit 1; \ - fi +# Option A (preferred when allowed): corepack +RUN corepack enable && corepack prepare "yarn@${YARN_VERSION#v}" --activate || \ + { echo "corepack unavailable; falling back to cached tarball"; \ + CACHED_YARN=./artifacts/yarn-${YARN_VERSION}.tar.gz; \ + test -f "${CACHED_YARN}" && npm install -g "${CACHED_YARN}"; }
24-28: Make shell compatible and align Yarn config file.Use POSIX “.” (or set SHELL to bash) and ensure we copy .yarnrc.yml for Yarn 3.
-# use dependencies provided by Cachito -RUN test -d ${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps || exit 1; \ - cp -f $REMOTE_SOURCES_DIR/cachito-gomod-with-deps/app/{.npmrc,.yarnrc,yarn.lock,registry-ca.pem} . \ - && source ${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/cachito.env \ - && yarn install --immutable && yarn build:all +# use dependencies provided by Cachito +SHELL ["/bin/bash", "-euo", "pipefail", "-c"] +RUN test -d "${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps"; \ + cp -f "${REMOTE_SOURCES_DIR}"/cachito-gomod-with-deps/app/{.npmrc,.yarnrc.yml,yarn.lock,registry-ca.pem} .; \ + source "${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/cachito.env"; \ + yarn install --immutable && yarn build:all
36-41: Harden runtime: CA certs and non-root execution.ubi-micro may lack CA certs; also run as non-root for OCP.
FROM registry.ci.openshift.org/ocp/ubi-micro:9 +RUN microdnf -y install ca-certificates && update-ca-trust && microdnf clean all COPY --from=ui-build /app/apps/assisted-disconnected-ui/build /app/proxy/dist COPY --from=proxy-build /app/assisted-disconnected-ui /app/proxy WORKDIR /app/proxy EXPOSE 8080 -CMD ./assisted-disconnected-ui +USER 1001 +CMD ["./assisted-disconnected-ui"]
10-11: Optional: keep memory cap but confirm builder limits.8192 MB may exceed CI quotas; confirm with ART CI limits.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/assisted-disconnected-ui/Containerfile.ocp.art(1 hunks)
pawanpinjarkar
force-pushed
the
update-dockerfile-to-provide-dependencies-from-remote-source
branch
from
5fd8298 to
dc80696
Compare
|
@pawanpinjarkar: This pull request references AGENT-1261 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 sub-task to target the "4.21.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. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/assisted-disconnected-ui/Containerfile.ocp (1)
49-53: Run as non-root in the final image and fix ownershipHarden runtime by dropping root and ensuring files are owned by the runtime UID.
-COPY --from=ui-build /app/apps/assisted-disconnected-ui/build /app/proxy/dist -COPY --from=proxy-build /app/assisted-disconnected-ui /app/proxy +COPY --chown=1001:0 --from=ui-build /app/apps/assisted-disconnected-ui/build /app/proxy/dist +COPY --chown=1001:0 --from=proxy-build /app/assisted-disconnected-ui /app/proxy WORKDIR /app/proxy EXPOSE 8080 +USER 1001:0 CMD ./assisted-disconnected-ui
🧹 Nitpick comments (3)
apps/assisted-disconnected-ui/Containerfile.ocp (3)
31-34: Be resilient to Yarn config file name differencesCachito-provided app may have
.yarnrc.yml(Yarn Berry) instead of.yarnrc. Copy whichever exists to avoid brittle failures.-RUN test -d ${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps || exit 1; \ - cp -f $REMOTE_SOURCES_DIR/cachito-gomod-with-deps/app/{.npmrc,.yarnrc,yarn.lock,registry-ca.pem} . \ +RUN test -d ${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps || exit 1; \ + for f in .npmrc .yarnrc .yarnrc.yml yarn.lock registry-ca.pem; do \ + [ -f "${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/app/${f}" ] && cp -f "${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/app/${f}" .; \ + done \
20-21: Remove redundant USER directiveYou’re already root from Line 12; the extra
USER 0is redundant.-USER 0 ARG YARN_VERSION=v3.4.1
46-46: Tighten Go build for smaller, more reproducible binaryAdd
-trimpathand-buildvcs=falseand strip symbols.- go build -o assisted-disconnected-ui . + go build -trimpath -buildvcs=false -ldflags "-s -w" -o assisted-disconnected-ui .
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/assisted-disconnected-ui/Containerfile.ocp(1 hunks)
| ARG YARN_VERSION=v3.4.1 | ||
| RUN CACHED_YARN=./artifacts/yarn-${YARN_VERSION}.tar.gz; \ | ||
| if [ -f ${CACHED_YARN} ]; then \ | ||
| npm install -g ${CACHED_YARN}; \ | ||
| else \ | ||
| echo "need yarn at ${CACHED_YARN}"; \ | ||
| exit 1; \ | ||
| fi |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Yarn bootstrap likely incorrect for Yarn 3; clarify target and use an offline-safe flow
If the intent is Yarn 3, installing via npm -g won’t work and --immutable won’t be recognized by Yarn 1. Please pick one of the two supported paths below.
Option A — Yarn 3 (offline, via local .cjs):
-ARG YARN_VERSION=v3.4.1
-RUN CACHED_YARN=./artifacts/yarn-${YARN_VERSION}.tar.gz; \
- if [ -f ${CACHED_YARN} ]; then \
- npm install -g ${CACHED_YARN}; \
- else \
- echo "need yarn at ${CACHED_YARN}"; \
- exit 1; \
- fi
+ARG YARN_VERSION=3.4.1
+RUN YARN_CJS=./artifacts/yarn-${YARN_VERSION}.cjs; \
+ if [ -f "${YARN_CJS}" ]; then \
+ node "${YARN_CJS}" --version; \
+ else \
+ echo "need yarn at ${YARN_CJS}"; \
+ exit 1; \
+ fi
@@
- && yarn install --immutable && yarn build:all
+ && node "./artifacts/yarn-${YARN_VERSION}.cjs" install --immutable \
+ && node "./artifacts/yarn-${YARN_VERSION}.cjs" build:allOption B — Yarn 1 (classic, offline tarball from npm):
-ARG YARN_VERSION=v3.4.1
+ARG YARN_VERSION=1.22.22
-RUN CACHED_YARN=./artifacts/yarn-${YARN_VERSION}.tar.gz; \
- if [ -f ${CACHED_YARN} ]; then \
- npm install -g ${CACHED_YARN}; \
+RUN CACHED_YARN=./artifacts/yarn-${YARN_VERSION}.tgz; \
+ if [ -f "${CACHED_YARN}" ]; then \
+ npm install -g "${CACHED_YARN}"; \
else \
echo "need yarn at ${CACHED_YARN}"; \
exit 1; \
fi
@@
- && yarn install --immutable && yarn build:all
+ && yarn install --frozen-lockfile && yarn build:allAlso applies to: 33-34
Clarify Yarn version and use offline-safe installation
The current bootstrap uses npm install -g on a .tar.gz, which won’t work for Yarn 3 and mismatches Yarn 1’s tarball format. Choose one of the following offline-safe flows:
Option A — Yarn 3 via single .cjs release
- ARG YARN_VERSION=v3.4.1
- RUN CACHED_YARN=./artifacts/yarn-${YARN_VERSION}.tar.gz; \
- if [ -f ${CACHED_YARN} ]; then \
- npm install -g ${CACHED_YARN}; \
+ ARG YARN_VERSION=3.4.1
+ RUN YARN_CJS=./artifacts/yarn-${YARN_VERSION}.cjs; \
+ if [ -f "${YARN_CJS}" ]; then \
+ node "${YARN_CJS}" --version; \
else \
echo "need yarn at ${YARN_CJS}"; \
exit 1; \
fi
@@
- && yarn install --immutable && yarn build:all
+ && node "./artifacts/yarn-${YARN_VERSION}.cjs" install --immutable \
+ && node "./artifacts/yarn-${YARN_VERSION}.cjs" build:all• Yarn 3 releases are distributed as .cjs files, not npm tarballs (git.marussy.com)
• Use --immutable (with alias --frozen-lockfile) for lockfile integrity (yarnpkg.com, stackoverflow.com)
Option B — Yarn 1 (Classic) via .tgz tarball
- ARG YARN_VERSION=v3.4.1
+ ARG YARN_VERSION=1.22.22
- RUN CACHED_YARN=./artifacts/yarn-${YARN_VERSION}.tar.gz; \
- if [ -f ${CACHED_YARN} ]; then \
- npm install -g ${CACHED_YARN}; \
+ RUN CACHED_YARN=./artifacts/yarn-${YARN_VERSION}.tgz; \
+ if [ -f "${CACHED_YARN}" ]; then \
+ npm install -g "${CACHED_YARN}"; \
else \
echo "need yarn at ${CACHED_YARN}"; \
exit 1; \
fi
@@
- && yarn install --immutable && yarn build:all
+ && yarn install --frozen-lockfile && yarn build:all• Yarn 1 publishes as .tgz tarballs (classic.yarnpkg.com)
• Use --frozen-lockfile for reproducible installs (classic.yarnpkg.com)
Also apply these changes around lines 33–34.
📝 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.
| ARG YARN_VERSION=v3.4.1 | |
| RUN CACHED_YARN=./artifacts/yarn-${YARN_VERSION}.tar.gz; \ | |
| if [ -f ${CACHED_YARN} ]; then \ | |
| npm install -g ${CACHED_YARN}; \ | |
| else \ | |
| echo "need yarn at ${CACHED_YARN}"; \ | |
| exit 1; \ | |
| fi | |
| ARG YARN_VERSION=3.4.1 | |
| RUN YARN_CJS=./artifacts/yarn-${YARN_VERSION}.cjs; \ | |
| if [ -f "${YARN_CJS}" ]; then \ | |
| node "${YARN_CJS}" --version; \ | |
| else \ | |
| echo "need yarn at ${YARN_CJS}"; \ | |
| exit 1; \ | |
| fi \ | |
| && node "./artifacts/yarn-${YARN_VERSION}.cjs" install --immutable \ | |
| && node "./artifacts/yarn-${YARN_VERSION}.cjs" build:all |
🤖 Prompt for AI Agents
In apps/assisted-disconnected-ui/Containerfile.ocp around lines 21–28 (and also
adjust the related install steps around lines 33–34), the Dockerfile attempts to
install a .tar.gz with `npm install -g`, which mismatches Yarn 3 distribution
format and is not offline-safe; update the bootstrap to one of two offline-safe
flows: Option A — for Yarn 3 set ARG to the .cjs filename (e.g.,
YARN_FILE=.yarn/releases/yarn-${YARN_VERSION}.cjs), copy that .cjs into the
image and install by placing the .cjs at /usr/local/bin/yarn (or create an
executable wrapper that runs `node /path/to/yarn-<ver>.cjs`) and run yarn
installs with `yarn install --immutable` (alias for frozen lockfile); Option B —
for Yarn 1 keep a .tgz classic tarball, install it with `npm install -g
./artifacts/yarn-${YARN_VERSION}.tgz` and run `yarn install --frozen-lockfile`;
pick the correct option, update ARG names/filenames accordingly, and replace the
current conditional install and subsequent install flags at lines ~33–34 to use
the corresponding frozen/immutable flag.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
@pawanpinjarkar: This pull request references AGENT-1261 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 sub-task to target the "4.21.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. |
|
@pawanpinjarkar: This pull request references AGENT-1261 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 sub-task to target the "4.21.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. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/assisted-disconnected-ui/Containerfile.ocp (2)
33-37: Make shell POSIX-safe and match Yarn flags to chosen major version
- Use POSIX “.” instead of “source”.
- If adopting Yarn 1 (per above), use --frozen-lockfile, not --immutable.
-RUN test -d ${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps || exit 1; \ - cp -f $REMOTE_SOURCES_DIR/cachito-gomod-with-deps/app/{.npmrc,.yarnrc,yarn.lock,registry-ca.pem} . \ - && source ${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/cachito.env \ - && yarn install --immutable && yarn build:all +RUN test -d "${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps" || exit 1; \ + cp -f "${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/app"/{.npmrc,.yarnrc,yarn.lock,registry-ca.pem} . \ + && . "${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/cachito.env" \ + && yarn install --frozen-lockfile && yarn build:all
8-17: Fix Yarn bootstrap: wrong artifact type for Yarn 3 and unconditional failure
- v3.4.1 is a Berry release (single .cjs), not an npm-installable tarball; the current flow won’t work.
- The else branch installs something and then exits 1, making builds always fail when cache is missing.
Apply one of the following. Option A aligns with the repo signals here (.yarnrc, yarn.lock): Yarn Classic.
Option A — Yarn 1 (classic, offline-friendly + online fallback)
-ARG YARN_VERSION=v3.4.1 +ARG YARN_VERSION=v1.22.22 @@ -RUN CACHED_YARN=./artifacts/yarn-${YARN_VERSION}.tar.gz; \ - if [ -f ${CACHED_YARN} ]; then \ - npm install -g ${CACHED_YARN}; \ - else \ - npm install https://github.com/yarnpkg/yarn/releases/download/${YARN_VERSION}/yarn-${YARN_VERSION}.tar.gz; \ - exit 1; \ - fi +RUN CACHED_YARN=./artifacts/yarn-${YARN_VERSION}.tgz; \ + if [ -f "${CACHED_YARN}" ]; then \ + npm install -g "${CACHED_YARN}"; \ + else \ + npm install -g "https://github.com/yarnpkg/yarn/releases/download/${YARN_VERSION}/yarn-${YARN_VERSION}.tar.gz"; \ + fiIf you truly need Yarn 3, switch to the .cjs bootstrap and invoke via node (and update the install flags accordingly). I can provide that variant if preferred.
🧹 Nitpick comments (2)
apps/assisted-disconnected-ui/Containerfile.ocp (2)
39-66: Final stage looks correct; minor nits onlyRuntime layout, workdir, and command are consistent. Consider adding non-root USER if compatible with the binary, and OCI labels later.
10-17: Optional: support both offline Cachito and upstream online builds cleanlyIf Cachito isn’t present, fall back to the online flow instead of failing. This keeps upstream builds working while ART uses REMOTE_SOURCES.
-RUN test -d "${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps" || exit 1; \ - cp -f "${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/app"/{.npmrc,.yarnrc,yarn.lock,registry-ca.pem} . \ - && . "${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/cachito.env" \ - && yarn install --frozen-lockfile && yarn build:all +RUN if [ -d "${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps" ]; then \ + cp -f "${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/app"/{.npmrc,.yarnrc,yarn.lock,registry-ca.pem} . \ + && . "${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/cachito.env" \ + && yarn install --frozen-lockfile; \ + else \ + echo "Cachito deps not found; proceeding with online install"; \ + yarn install --frozen-lockfile; \ + fi \ + && yarn build:allAlso applies to: 33-37
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/assisted-disconnected-ui/Containerfile.ocp(1 hunks)
🔇 Additional comments (6)
apps/assisted-disconnected-ui/Containerfile.ocp (6)
19-25: ARG defaults and comments look goodClear docs and sane defaults (once the path is fixed). No issues.
28-32: UI build context setup LGTMCopy with chown, memory limit, and git safe.directory are appropriate.
41-46: Go builder base and ARG reuse LGTMPinning to golang 1.23 builder and reusing REMOTE_SOURCES_DIR is fine.
60-66: Entrypoint/ports LGTMEXPOSE 8080 and running the built binary from WORKDIR is consistent.
1-7: Stage headers and base image are fineNo action needed here.
10-17: Clarify Yarn major version for assisted-disconnected-ui
No.yarnrc.yml,.yarnrcoryarn.lockfound underapps/assisted-disconnected-ui, so it’s unclear whether this Containerfile should bootstrap Yarn 1 (classic) or Yarn 3 (berry). Please confirm the intended major version before merging (affects lines 10–17 and 33–37).
| COPY --from=ui-build ${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/app/ /app | ||
| USER 0 | ||
| RUN go build | ||
| # Build deterministically and name the output explicitly | ||
| ENV CGO_ENABLED=0 | ||
| RUN source ${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/cachito.env; \ | ||
| if [ -d vendor ]; then export GOFLAGS="-mod=vendor"; fi; \ | ||
| go build -o assisted-disconnected-ui . |
There was a problem hiding this comment.
🛠️ Refactor suggestion
proxy-build sources cachito.env without copying it into this stage
Only /app is copied from ui-build; /remote-sources/.../cachito.env is missing, so “source …/cachito.env” will fail.
COPY --from=ui-build ${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/app/ /app
+COPY --from=ui-build ${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/cachito.env /tmp/cachito.env
@@
-RUN source ${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/cachito.env; \
+RUN . /tmp/cachito.env; \
if [ -d vendor ]; then export GOFLAGS="-mod=vendor"; fi; \
go build -o assisted-disconnected-ui .📝 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.
| COPY --from=ui-build ${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/app/ /app | |
| USER 0 | |
| RUN go build | |
| # Build deterministically and name the output explicitly | |
| ENV CGO_ENABLED=0 | |
| RUN source ${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/cachito.env; \ | |
| if [ -d vendor ]; then export GOFLAGS="-mod=vendor"; fi; \ | |
| go build -o assisted-disconnected-ui . | |
| COPY --from=ui-build ${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/app/ /app | |
| COPY --from=ui-build ${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/cachito.env /tmp/cachito.env | |
| USER 0 | |
| # Build deterministically and name the output explicitly | |
| ENV CGO_ENABLED=0 | |
| RUN . /tmp/cachito.env; \ | |
| if [ -d vendor ]; then export GOFLAGS="-mod=vendor"; fi; \ | |
| go build -o assisted-disconnected-ui . |
🤖 Prompt for AI Agents
In apps/assisted-disconnected-ui/Containerfile.ocp around lines 49-55 the RUN
tries to source ${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/cachito.env but
that file is not copied into this stage (only /app is copied), so sourcing will
fail; fix by copying the cachito.env from the ui-build stage into the same path
before using it (e.g., add a COPY --from=ui-build for the cachito.env into
${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/), or alternatively guard the RUN
to only source the file if it exists (e.g., check -f before sourcing) — choose
one approach and implement it so the build no longer errors when sourcing
cachito.env.
There was a problem hiding this comment.
cachito.env is referenced from https://github.com/openshift/networking-console-plugin/blob/release-4.20/Dockerfile.art#L23 but I am not sure if its needed TBH.
There was a problem hiding this comment.
in the newer cachi2 system.
My analysis suggests two approaches:
Option 1: Remove cachito.env sourcing entirely (Recommended)
Since your Go build:
- Uses vendor mode (
if [ -d vendor ]; then export GOFLAGS="-mod=vendor") - Has
CGO_ENABLED=0(pure Go build) - Doesn't seem to require network access during build
The cachito.env sourcing is likely unnecessary. You could simplify to:
RUN if [ -d vendor ]; then export GOFLAGS="-mod=vendor"; fi; \
go build -o assisted-disconnected-ui .Option 2: Make it conditional (Safer if unsure)
RUN if [ -f ${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/cachito.env ]; then \
source ${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/cachito.env; \
fi; \
if [ -d vendor ]; then export GOFLAGS="-mod=vendor"; fi; \
go build -o assisted-disconnected-ui .Given that you're using vendored dependencies and this is a simple Go binary build, I'd recommend Option 1 - just remove the cachito.env sourcing entirely. The networking-console-plugin reference might be from an older pattern that's no longer needed.
What's your preference?
🐰 curious ears perk up, investigating the mystery of the potentially unnecessary env file
✏️ Learnings added
Learnt from: pawanpinjarkar
PR: openshift-assisted/assisted-installer-ui#3152
File: apps/assisted-disconnected-ui/Containerfile.ocp:49-55
Timestamp: 2025-09-02T22:43:45.127Z
Learning: In OpenShift ART builds with Cachito, the cachito.env file sourcing may not be necessary for Go builds that use vendor mode, especially with the newer cachi2 system which automatically handles environment setup.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
@pawanpinjarkar: This pull request references AGENT-1261 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 sub-task to target the "4.21.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. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/assisted-disconnected-ui/Containerfile.ocp (2)
49-55: Remove/guard unnecessary cachito.env usage in Go build (and UI stage)With vendor mode and cachi2, sourcing cachito.env is typically unnecessary and it isn’t copied into this stage, causing failures. Either remove it or guard it.
-RUN . ${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/cachito.env; \ - if [ -d vendor ]; then export GOFLAGS="-mod=vendor"; fi; \ +RUN if [ -d vendor ]; then export GOFLAGS="-mod=vendor"; fi; \ go build -o assisted-disconnected-ui .(Optional guard instead of removal):
-RUN . ${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/cachito.env; \ +RUN [ -f ${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/cachito.env ] && . ${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/cachito.env || true; \ if [ -d vendor ]; then export GOFLAGS="-mod=vendor"; fi; \ go build -o assisted-disconnected-ui .
8-17: Broken Yarn bootstrap: version/format mismatch and guaranteed failure path
- Using v3 (Berry) with npm -g tarball is incorrect; Yarn 3 ships as a single .cjs file.
- If cached artifact is missing, the current else installs from the internet and then unconditionally exits 1, so builds always fail.
- Pick one flow and align flags and files.
Option A — Yarn 3 (Berry), offline via .cjs committed/provided by Cachito:
-ARG YARN_VERSION=v3.4.1 -RUN CACHED_YARN=./artifacts/yarn-${YARN_VERSION}.tar.gz; \ - if [ -f ${CACHED_YARN} ]; then \ - npm install -g ${CACHED_YARN}; \ - else \ - npm install https://github.com/yarnpkg/yarn/releases/download/${YARN_VERSION}/yarn-${YARN_VERSION}.tar.gz; \ - exit 1; \ - fi +ARG YARN_VERSION=3.4.1 +RUN set -euo pipefail; \ + YARN_CJS="${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/app/.yarn/releases/yarn-${YARN_VERSION}.cjs"; \ + if [ -f "${YARN_CJS}" ]; then \ + install -D -m0755 "${YARN_CJS}" /usr/local/bin/yarn.cjs; \ + printf '#!/usr/bin/env sh\nexec node /usr/local/bin/yarn.cjs "$@"\n' > /usr/local/bin/yarn; \ + chmod +x /usr/local/bin/yarn; \ + yarn --version; \ + else \ + echo "Missing ${YARN_CJS}. Provide Yarn release via Cachito."; \ + exit 1; \ + fiOption B — Yarn 1 (Classic), offline-first, with controlled online fallback:
-ARG YARN_VERSION=v3.4.1 +ARG YARN_VERSION=v1.22.22 +ARG ALLOW_ONLINE_YARN=0 -RUN CACHED_YARN=./artifacts/yarn-${YARN_VERSION}.tar.gz; \ - if [ -f ${CACHED_YARN} ]; then \ - npm install -g ${CACHED_YARN}; \ - else \ - npm install https://github.com/yarnpkg/yarn/releases/download/${YARN_VERSION}/yarn-${YARN_VERSION}.tar.gz; \ - exit 1; \ - fi +RUN set -euo pipefail; \ + CACHED_YARN="${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/app/artifacts/yarn-${YARN_VERSION}.tar.gz"; \ + if [ -f "${CACHED_YARN}" ]; then \ + npm install -g "${CACHED_YARN}"; \ + elif [ "${ALLOW_ONLINE_YARN}" = "1" ]; then \ + npm install -g "https://github.com/yarnpkg/yarn/releases/download/${YARN_VERSION}/yarn-${YARN_VERSION}.tar.gz"; \ + else \ + echo "Need ${CACHED_YARN} (online fallback disabled)"; \ + exit 1; \ + fi
🧹 Nitpick comments (2)
apps/assisted-disconnected-ui/Containerfile.ocp (2)
26-26: Prefer brace expansion for ARGs in COPYSafer and consistent with other usages.
-COPY $REMOTE_SOURCES $REMOTE_SOURCES_DIR +COPY ${REMOTE_SOURCES} ${REMOTE_SOURCES_DIR}
6-6: Drop root after setup to follow OpenShift restricted SCCRun build steps that require root, but switch back to a non-root UID for subsequent steps and in the final image. Ensure /app (and later /app/proxy) is owned by 1001.
Outside this hunk, add in final stage:
RUN chown -R 1001:0 /app/proxy USER 1001
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/assisted-disconnected-ui/Containerfile.ocp(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: pawanpinjarkar
PR: openshift-assisted/assisted-installer-ui#3152
File: apps/assisted-disconnected-ui/Containerfile.ocp:49-55
Timestamp: 2025-09-02T22:43:45.127Z
Learning: In OpenShift ART builds with Cachito, the cachito.env file sourcing may not be necessary for Go builds that use vendor mode, especially with the newer cachi2 system which automatically handles environment setup.
📚 Learning: 2025-09-02T22:43:45.127Z
Learnt from: pawanpinjarkar
PR: openshift-assisted/assisted-installer-ui#3152
File: apps/assisted-disconnected-ui/Containerfile.ocp:49-55
Timestamp: 2025-09-02T22:43:45.127Z
Learning: In OpenShift ART builds with Cachito, the cachito.env file sourcing may not be necessary for Go builds that use vendor mode, especially with the newer cachi2 system which automatically handles environment setup.
Applied to files:
apps/assisted-disconnected-ui/Containerfile.ocp
| # use dependencies provided by Cachito | ||
| RUN test -d ${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps || exit 1; \ | ||
| cp -f $REMOTE_SOURCES_DIR/cachito-gomod-with-deps/app/{.npmrc,.yarnrc,yarn.lock,registry-ca.pem} . \ | ||
| && . ${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/cachito.env \ | ||
| && yarn install --immutable && yarn build:all |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Align dependency files and install flags with chosen Yarn major; avoid brittle brace copy
- If you pick Yarn 3, configs are typically yarnrc.yml (not .yarnrc) and the correct flag is --immutable.
- If you pick Yarn 1, keep .yarnrc and use --frozen-lockfile.
- Also guard missing files and avoid brace expansion that fails if one file is absent.
Option A — Yarn 3:
-RUN test -d ${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps || exit 1; \
- cp -f $REMOTE_SOURCES_DIR/cachito-gomod-with-deps/app/{.npmrc,.yarnrc,yarn.lock,registry-ca.pem} . \
- && . ${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/cachito.env \
- && yarn install --immutable && yarn build:all
+RUN set -euo pipefail; \
+ test -d "${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps" || exit 1; \
+ for f in .npmrc yarn.lock yarnrc.yml registry-ca.pem; do \
+ [ -f "${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/app/${f}" ] && \
+ cp -f "${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/app/${f}" . || true; \
+ done; \
+ yarn install --immutable && yarn build:allOption B — Yarn 1:
-RUN test -d ${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps || exit 1; \
- cp -f $REMOTE_SOURCES_DIR/cachito-gomod-with-deps/app/{.npmrc,.yarnrc,yarn.lock,registry-ca.pem} . \
- && . ${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/cachito.env \
- && yarn install --immutable && yarn build:all
+RUN set -euo pipefail; \
+ test -d "${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps" || exit 1; \
+ for f in .npmrc .yarnrc yarn.lock registry-ca.pem; do \
+ [ -f "${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/app/${f}" ] && \
+ cp -f "${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/app/${f}" . || true; \
+ done; \
+ yarn install --frozen-lockfile && yarn build:all🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1. List Yarn config files to detect .yarnrc vs .yarnrc.yml
find . -maxdepth 3 -type f -name ".yarnrc*" -print
# 2. Inspect package.json for packageManager field (Node 16+ spec for Yarn)
rg -n '"packageManager"' -g 'package.json' || true
# 3. Scan for yarn install invocations to see which flags are already used
rg -n 'yarn install' -n .Length of output: 845
Use Yarn 3 config and guard missing files when copying Cachito deps
Project uses Yarn 3 (packageManager: "yarn@3.4.1" and .yarnrc.yml present), so copy .yarnrc.yml (not .yarnrc) and keep --immutable. Wrap copies in a loop to avoid brace‐expansion failures and retain sourcing of cachito.env:
-RUN test -d ${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps || exit 1; \
- cp -f $REMOTE_SOURCES_DIR/cachito-gomod-with-deps/app/{.npmrc,.yarnrc,yarn.lock,registry-ca.pem} . \
- && . ${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/cachito.env \
- && yarn install --immutable && yarn build:all
+RUN set -euo pipefail; \
+ test -d "${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps" || exit 1; \
+ for f in .npmrc yarn.lock .yarnrc.yml registry-ca.pem; do \
+ [ -f "${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/app/${f}" ] && \
+ cp -f "${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/app/${f}" .; \
+ done; \
+ . "${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/cachito.env"; \
+ yarn install --immutable && yarn build:all📝 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.
| # use dependencies provided by Cachito | |
| RUN test -d ${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps || exit 1; \ | |
| cp -f $REMOTE_SOURCES_DIR/cachito-gomod-with-deps/app/{.npmrc,.yarnrc,yarn.lock,registry-ca.pem} . \ | |
| && . ${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/cachito.env \ | |
| && yarn install --immutable && yarn build:all | |
| # use dependencies provided by Cachito | |
| RUN set -euo pipefail; \ | |
| test -d "${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps" || exit 1; \ | |
| for f in .npmrc yarn.lock .yarnrc.yml registry-ca.pem; do \ | |
| [ -f "${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/app/${f}" ] && \ | |
| cp -f "${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/app/${f}" .; \ | |
| done; \ | |
| . "${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/cachito.env"; \ | |
| yarn install --immutable && yarn build:all |
🤖 Prompt for AI Agents
In apps/assisted-disconnected-ui/Containerfile.ocp around lines 33 to 37, the
Dockerfile copies the wrong Yarn config and uses brace expansion which can fail
and abort the build; update it to copy .yarnrc.yml (Yarn v3) instead of .yarnrc,
keep the yarn install --immutable and yarn build:all, and replace the single cp
with a guarded loop that checks each expected file (e.g., .yarnrc.yml,
yarn.lock, registry-ca.pem, .npmrc if present) before copying to avoid failures
when some files are missing, while retaining the source of cachito.env prior to
running yarn commands.
|
@pawanpinjarkar: This pull request references AGENT-1261 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 sub-task to target the "4.21.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. |
|
Tip For best results, initiate chat on the files or code changes. @pawanpinjarkar Hello! I see you've mentioned me. How can I help you with this PR? I'm ready to assist with reviewing the changes, answering questions about the Dockerfile modifications, or anything else you need regarding the dependency sourcing updates. ฅ(•ㅅ•❀)ฅ |
|
@coderabbitai code review |
|
@pawanpinjarkar I'll conduct a comprehensive code review of your PR that updates dependency sourcing to remote sources for ART builds. ✅ Actions performedReview triggered.
|
|
A lot of noise here. Closed this and created another clean PR #3155 |
4 participants
Adds a new dockerfile for ART to manage dependencies from REMOTE_SOURCES and CACHED_YARN for the ART builds
Summary by CodeRabbit