Skip to content

AGENT-1261: Update dependency sourcing to remote #3152

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

Closed
pawanpinjarkar wants to merge 6 commits into from
Closed

AGENT-1261: Update dependency sourcing to remote #3152

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
dc80696
AGENT-1261: Update dependency sourcing to remote
pawanpinjarkar Sep 2, 2025
534f5be
review fixes
pawanpinjarkar Sep 2, 2025
987f5f2
Making the structure as close as Console repo's dockerfile
pawanpinjarkar Sep 2, 2025
89f4ae7
Use POSIX ‘.’ instead of ‘source’ to load cachito.env
pawanpinjarkar Sep 2, 2025
e4669cb
fix filename
pawanpinjarkar Sep 2, 2025
028616f
Updated base image, fixed unwanted cachito.env usage
pawanpinjarkar Sep 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 36 additions & 6 deletions apps/assisted-disconnected-ui/Containerfile.ocp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,49 @@
FROM registry.ci.openshift.org/ocp/builder:rhel-9-base-nodejs-openshift-4.19 AS ui-build

# The REMOTE_SOURCES value is set by the build system to indicate the location of the cachito-backed artifacts cache.
# As cachito might not be available in all environments, we need to make sure the value is set before trying to use it and
# that the COPY layer below doesn't fail. Setting it to be the Dockerfile itself is fairly safe, as it will always be
# available.
ARG REMOTE_SOURCES
Comment thread
pawanpinjarkar marked this conversation as resolved.
Outdated
ARG REMOTE_SOURCES_DIR=/remote-sources

COPY $REMOTE_SOURCES $REMOTE_SOURCES_DIR

Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
USER root

WORKDIR /app
COPY --chown=1001:0 / /app
RUN ls /app
COPY --chown=1001:0 . /app
ENV NODE_OPTIONS='--max-old-space-size=8192'
RUN git config --global --add safe.directory /app
RUN npm install -g corepack@0.24.1
Comment thread
pawanpinjarkar marked this conversation as resolved.
RUN yarn install --immutable && yarn build:all

# bootstrap yarn so we can install and run the other tools.
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}"; \
Comment thread
pawanpinjarkar marked this conversation as resolved.
Outdated
exit 1; \
fi

@coderabbitai coderabbitai Bot Sep 2, 2025 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 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:all

Option 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:all

Also 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.

Suggested change
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.

@pawanpinjarkar pawanpinjarkar Sep 2, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@rawagner @Ximinhan PTAL

@coderabbitai coderabbitai Bot Sep 2, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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!


# 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} . \
Comment thread
pawanpinjarkar marked this conversation as resolved.
Outdated
&& source ${REMOTE_SOURCES_DIR}/cachito-gomod-with-deps/cachito.env \
&& yarn install --immutable && yarn build:all
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Comment on lines +33 to +37

@coderabbitai coderabbitai Bot Sep 2, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 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:all

Option 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.

Suggested change
# 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.


FROM registry.ci.openshift.org/ocp/builder:rhel-9-golang-1.23-openshift-4.19 as proxy-build
ARG REMOTE_SOURCES_DIR=/remote-sources
WORKDIR /app
COPY apps/assisted-disconnected-ui/proxy /app
# Use Cachito-provided source + deps for offline build
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 .
Comment on lines +49 to +54

@coderabbitai coderabbitai Bot Sep 2, 2025 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

⚠️ Potential issue

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.

Suggested change
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.

@pawanpinjarkar pawanpinjarkar Sep 2, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@coderabbitai coderabbitai Bot Sep 2, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.


FROM registry.ci.openshift.org/ocp/ubi-micro:9
COPY --from=ui-build /app/apps/assisted-disconnected-ui/build /app/proxy/dist
Expand Down