Build from source some extensions that were previously bundled. - #775
Build from source some extensions that were previously bundled.#775rgrunber wants to merge 1 commit into
Conversation
|
Click here to review and test in web IDE: |
&message=Dev%20cluster%20(for%20maintainers)&logo=eclipseche&color=525C86&labelColor=FDB940)
📝 WalkthroughWalkthroughThe change adds four Git submodules and enables submodule checkout in both build workflows. Install hooks copy extension build scripts and update Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Checkout
participant NpmInstall
participant BuildScripts
participant ExtensionRepositories
Checkout->>ExtensionRepositories: fetch submodules
NpmInstall->>BuildScripts: run preinstall helpers
BuildScripts->>ExtensionRepositories: copy build scripts and update metadata
BuildScripts->>ExtensionRepositories: invoke extension builds
ExtensionRepositories-->>BuildScripts: generate build outputs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/build/npm/dirs.ts (1)
31-46: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winEnsure the npm cache contract matches these added dirs.
extensions/devfileis a TypeScript build target but has nopackage.json/package-lock.json, so it is exposed to the cache logic without the files it expects; either add a no-op package contract or move it out of this npm dir list. Similarly,extensions/js-profile-visualizercompiles nestedpackages/vscode-js-profile-table; if that package depends on its own install artifacts, add entries/lockfile handling for the nested package too.
[mantainability_and_code_quality]🤖 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 `@code/build/npm/dirs.ts` around lines 31 - 46, Update the npm directory configuration containing the listed extension paths so every cached build target has the package artifacts expected by the cache logic. For extensions/devfile, either add the required no-op package contract or remove it from the npm directory list; for extensions/js-profile-visualizer, include handling for the nested packages/vscode-js-profile-table package and its lockfile when required.
🤖 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 @.github/workflows/image-publish.yml:
- Around line 39-40: Update the actions/checkout configuration by adding
persist-credentials: false alongside submodules: true, ensuring credentials are
not retained in the workspace before the subsequent Docker build.
In `@code/build/copy-extension-build-scripts.js`:
- Around line 34-42: Update the missing-input checks in the copy flow, including
the checks for src, destDir, and jsDebugPkgPath, to throw an error immediately
instead of warning and continuing. Preserve the existing path-specific error
context while ensuring any absent required submodule directory, build script, or
js-debug package metadata causes the script to fail.
In `@code/build/extension-build-scripts/js-profile-visualizer.esbuild.ts`:
- Around line 14-23: Update the two tsc invocations in the build script so
catches do not unconditionally continue after compiler failures. Allow only
explicitly tolerated type-check errors, or validate that all expected out/esm
outputs were freshly generated before proceeding; otherwise propagate the
failure and prevent packaging an invalid extension.
---
Outside diff comments:
In `@code/build/npm/dirs.ts`:
- Around line 31-46: Update the npm directory configuration containing the
listed extension paths so every cached build target has the package artifacts
expected by the cache logic. For extensions/devfile, either add the required
no-op package contract or remove it from the npm directory list; for
extensions/js-profile-visualizer, include handling for the nested
packages/vscode-js-profile-table package and its lockfile when required.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 71a74af2-86ee-4e51-8b8e-a09ae7d4786e
📒 Files selected for processing (18)
.github/workflows/image-publish.yml.gitmodulesbuild/artifacts/artifacts.lock.yamlcode/build/copy-extension-build-scripts.jscode/build/extension-build-scripts/devfile.esbuild.tscode/build/extension-build-scripts/js-debug-companion.esbuild.tscode/build/extension-build-scripts/js-debug.esbuild.tscode/build/extension-build-scripts/js-profile-visualizer.esbuild.tscode/build/extension-build-scripts/js-profile-visualizer.webpack.override.jscode/build/gulpfile.extensions.tscode/build/npm/dirs.tscode/extensions/devfilecode/extensions/js-debugcode/extensions/js-debug-companioncode/extensions/js-profile-visualizercode/package.jsoncode/product.jsonpackage.json
💤 Files with no reviewable changes (2)
- code/product.json
- build/artifacts/artifacts.lock.yaml
| with: | ||
| submodules: true |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not persist the checkout token into the build workspace.
actions/checkout persists credentials by default. With submodules enabled, the token can remain in Git metadata available to the subsequent docker build ... . context. Set persist-credentials: false after checkout has fetched the submodules.
Proposed fix
with:
submodules: true
+ persist-credentials: false📝 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.
| with: | |
| submodules: true | |
| with: | |
| submodules: true | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.28.0)
[warning] 37-40: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 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 @.github/workflows/image-publish.yml around lines 39 - 40, Update the
actions/checkout configuration by adding persist-credentials: false alongside
submodules: true, ensuring credentials are not retained in the workspace before
the subsequent Docker build.
Source: Linters/SAST tools
| if (!fs.existsSync(src)) { | ||
| console.warn(`Warning: ${src} not found, skipping`); | ||
| continue; | ||
| } | ||
|
|
||
| if (!fs.existsSync(destDir)) { | ||
| console.warn(`Warning: ${destDir} not found, skipping`); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fail closed when required extension inputs are missing.
The script currently exits successfully when a required submodule directory, build script, or js-debug/package.json is absent. CI can therefore continue with stale scripts or unmodified metadata and produce a non-reproducible package. Throw an error instead of warning and continuing.
Proposed fix
if (!fs.existsSync(src)) {
- console.warn(`Warning: ${src} not found, skipping`);
- continue;
+ throw new Error(`Required build script not found: ${src}`);
}
if (!fs.existsSync(destDir)) {
- console.warn(`Warning: ${destDir} not found, skipping`);
- continue;
+ throw new Error(`Required extension directory not found: ${destDir}`);
}Apply the same fail-fast behavior when jsDebugPkgPath is missing.
Also applies to: 49-60
🤖 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 `@code/build/copy-extension-build-scripts.js` around lines 34 - 42, Update the
missing-input checks in the copy flow, including the checks for src, destDir,
and jsDebugPkgPath, to throw an error immediately instead of warning and
continuing. Preserve the existing path-specific error context while ensuring any
absent required submodule directory, build script, or js-debug package metadata
causes the script to fail.
21dd277 to
871ed3b
Compare
rgrunber
changed the title
|
Pull Request images published ✨ Editor amd64: quay.io/che-incubator-pull-requests/che-code:pr-775-amd64 |
|
Hi! I'm che-ai-assistant — I help with your pull requests. I check for new comments every 10m0s, so there may be a short delay before I respond. Available commands:
|
- devfile.vscode-devfile, ms-vscode.js-debug, js-debug-companion, js-profile-table - Create git submodules under extensions/ - Add extensions to code/build/npm/dirs.ts to trigger npm-install - Create .esbuild.ts to wrap compilation process of each extension Assisted-By: claude-opus-4-6 Signed-off-by: Roland Grunberg <rgrunber@redhat.com>
871ed3b to
2a9dd85
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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 @.github/workflows/pull-request-check.yml:
- Around line 57-58: Update the actions/checkout configuration in the workflow’s
with block to set persist-credentials to false, while preserving the existing
submodules setting, unless a later workflow step explicitly requires Git
authentication.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7c8e5e8d-04c6-4deb-ae75-67249fa6b6a1
📒 Files selected for processing (19)
.github/workflows/image-publish.yml.github/workflows/pull-request-check.yml.gitmodulesbuild/artifacts/artifacts.lock.yamlcode/build/copy-extension-build-scripts.jscode/build/extension-build-scripts/devfile.esbuild.tscode/build/extension-build-scripts/js-debug-companion.esbuild.tscode/build/extension-build-scripts/js-debug.esbuild.tscode/build/extension-build-scripts/js-profile-visualizer.esbuild.tscode/build/extension-build-scripts/js-profile-visualizer.webpack.override.jscode/build/gulpfile.extensions.tscode/build/npm/dirs.tscode/extensions/devfilecode/extensions/js-debugcode/extensions/js-debug-companioncode/extensions/js-profile-visualizercode/package.jsoncode/product.jsonpackage.json
💤 Files with no reviewable changes (2)
- code/product.json
- build/artifacts/artifacts.lock.yaml
🚧 Files skipped from review as they are similar to previous changes (11)
- code/package.json
- code/extensions/js-debug-companion
- code/build/extension-build-scripts/devfile.esbuild.ts
- code/build/extension-build-scripts/js-debug-companion.esbuild.ts
- code/extensions/js-profile-visualizer
- .gitmodules
- code/build/extension-build-scripts/js-profile-visualizer.webpack.override.js
- code/extensions/js-debug
- code/build/gulpfile.extensions.ts
- code/extensions/devfile
- package.json
| with: | ||
| submodules: true |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'persist-credentials|GITHUB_TOKEN|github.token' .github/workflows/pull-request-check.yml
rg -n '(^|/)\.dockerignore$|COPY[[:space:]]+\.' --hidden --glob '!.git/**' .
rg -n '(^|/)\.git(/|$)|git config|git clone|git submodule' build code .dockerignore 2>/dev/null || trueRepository: che-incubator/che-code
Length of output: 160
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- workflow ---'
sed -n '1,110p' .github/workflows/pull-request-check.yml
printf '%s\n' '--- relevant repository files ---'
git ls-files | rg '(^|/)(\.dockerignore|Dockerfile[^/]*|build/|code/)'
printf '%s\n' '--- dockerignore files ---'
find . -name .dockerignore -print
printf '%s\n' '--- Dockerfile context and git references ---'
rg -n --hidden --glob '!.git/**' '(^|/)\.git(/|$)|git config|git clone|git submodule|COPY[[:space:]]+\.' \
.github .dockerignore build code Dockerfile* 2>/dev/null || trueRepository: che-incubator/che-code
Length of output: 50379
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- workflow checkout and build steps ---'
rg -n -C 8 'actions/checkout|docker build|submodules|persist-credentials' .github/workflows/pull-request-check.yml
printf '%s\n' '--- tracked Docker-related files ---'
git ls-files | rg '(^|/)(Dockerfile[^/]*|\.dockerignore)$|(^|/)(build|code)/.*Dockerfile'
printf '%s\n' '--- root and tracked dockerignore status ---'
if [ -e .dockerignore ]; then
sed -n '1,160p' .dockerignore
else
printf '%s\n' 'No root .dockerignore'
fi
git ls-files | rg '(^|/)\.dockerignore$' || true
printf '%s\n' '--- Dockerfile instructions using repository context ---'
while IFS= read -r f; do
printf '%s\n' "--- $f"
rg -n '^(FROM|COPY|ADD|RUN).*' "$f"
done < <(git ls-files | rg '(^|/)(Dockerfile[^/]*|.*Dockerfile)$')Repository: che-incubator/che-code
Length of output: 4909
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- all checkout inputs and post-checkout Git use ---'
rg -n -C 4 'uses:[[:space:]]*actions/checkout|persist-credentials|git[[:space:]]|GITHUB_TOKEN|github\.token' \
.github/workflows/pull-request-check.yml
printf '%s\n' '--- Dockerfile instructions ---'
for f in build/dockerfiles/*.Dockerfile; do
printf '%s\n' "--- $f"
rg -n '^(FROM|COPY|ADD|RUN)[[:space:]]' "$f"
done
printf '%s\n' '--- exact build-context exclusion ---'
cat -n .dockerignore
printf '%s\n' '--- read-only context model ---'
python3 - <<'PY'
from pathlib import Path
ignore = Path(".dockerignore").read_text().splitlines()
print("git_excluded:", any(line.strip() in {".git", "**/.git", ".git/**"} for line in ignore))
for p in sorted(Path("build/dockerfiles").glob("*.Dockerfile")):
lines = p.read_text().splitlines()
context_ops = [f"{i}: {line}" for i, line in enumerate(lines, 1)
if line.startswith(("COPY ", "ADD "))]
print(p, context_ops)
PYRepository: che-incubator/che-code
Length of output: 12552
Disable checkout credential persistence.
actions/checkout stores the token in local Git configuration by default. This workflow executes repository scripts from the pull request, which can read that configuration. Add persist-credentials: false unless a later step requires Git authentication.
🧰 Tools
🪛 zizmor (1.29.0)
[warning] 55-58: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 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 @.github/workflows/pull-request-check.yml around lines 57 - 58, Update the
actions/checkout configuration in the workflow’s with block to set
persist-credentials to false, while preserving the existing submodules setting,
unless a later workflow step explicitly requires Git authentication.
Source: Linters/SAST tools
|
Pull Request images published ✨ Editor amd64: quay.io/che-incubator-pull-requests/che-code:pr-775-amd64 |
2 participants
Assisted-By: claude-opus-4-6
I tested this by publishing to https://quay.io/repository/rgrunber/che-code?tab=tags and then running that editor image. The extensions seem to be bundled as part of the editor.
The only thing I need to verify is that the contents of the published vsix in the marketplace match the contents (roughly) of what the build generates. I think the
.vscodeignoreincludes too many things which would result in a larger installation.Summary by CodeRabbit
New Features
Bug Fixes
Build Improvements